Skip to content

Remove redundant escapes in regular expressions #24470

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

hyeonisism
Copy link
Contributor

Remove redundant character escape in RegExp

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Feb 3, 2020
@sbrannen
Copy link
Member

sbrannen commented Feb 3, 2020

Whether or not it makes sense to remove those unnecessary escape sequences is (in my opinion) a matter of taste. In other words, it's technically correct to omit the escaping for a closing brace (and more concise), but many people are accustomed to escaping all special characters (opening and closing).

In any case, was there a reason you did not remove all such unnecessary escaping in the GLOB_PATTERN?

@sbrannen sbrannen added in: core Issues in core modules (aop, beans, core, context, expression) type: task A general task labels Feb 3, 2020
@hyeonisism
Copy link
Contributor Author

hyeonisism commented Feb 3, 2020

@sbrannen
When I revised the code, there was an unnecessary RegExp and I deleted it.

But listening to your opinion, I seem to have tried to break down the style that many have.

However, I think there is progress if technological pursuit is correct. What is your opinion?

Apart from the earlier story, GLOB_PATTERN part is a mistake I missed.

Thank you for sharing good idea.

@rstoyanchev rstoyanchev self-assigned this Feb 3, 2020
@rstoyanchev rstoyanchev removed the status: waiting-for-triage An issue we've not yet triaged or decided on label Feb 3, 2020
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Feb 3, 2020

We discussed this within the team and decided to go ahead with this change in part to make the expressions more readable, and in part to avoid IDE warnings that add to the noise. To make matters worse, IntelliJ flags this warning while Eclipse doesn't. This means that while @SuppressWarnings would work in IntelliJ, in Eclipse it would be flagged as unnecessary. All of these outcomes only add more noise.

So we are okay with the change, but it needs to be applied consistently eveywhere. These are other classes where this exists:

AbstractHttpSendingTransportHandler
ClassPathResourceTests
CustomizableTraceInterceptor
MessageBrokerBeanDefinitionParserTests
RedirectView
RegexPathElement
TestContextResourceUtils
UriComponents
UriComponentsBuilder
WebSocketMessageBrokerConfigurationSupportTests

Let me know if you are you going to make those changes.

@hyeonisism
Copy link
Contributor Author

@rstoyanchev
You mean If I wanted to, I have to apply this whole thing?
I'd like to. Can I proceed from this Branch?

@rstoyanchev
Copy link
Contributor

Yes if you'd like to, please go ahead and make those updates in this branch. I'll update the PR title accordingly to reflect the expanded scope.

@rstoyanchev rstoyanchev changed the title Remove redundant character escape in AntPathMatcher class Remove redundant escapes in regular expressions Feb 3, 2020
@rstoyanchev rstoyanchev added this to the 5.2.4 milestone Feb 3, 2020
@rstoyanchev
Copy link
Contributor

Thanks, I think the commits can be squashed. Logically it's one change applied everywhere.

Update copy right year

Remove unnecessary escaping

Remove unnecessary escaping in AbstractHttpSendingTransportHandler class

Remove unnecessary escaping in ClassPathResourceTests

Remove unnecessary escaping in CustomizableTraceInterceptor

Remove unnecessary escaping in MessageBrokerBeanDefinitionParserTests

Remove unnecessary escaping in RedirectView

Remove unnecessary escaping in RegexPathElement

Remove unnecessary escaping in TestContextResourceUtils

Remove unnecessary escaping in UriComponents

Remove unnecessary escaping in UriComponentsBuilder

Remove unnecessary escaping in WebSocketMessageBrokerConfigurationSupportTests
@hyeonisism hyeonisism force-pushed the Remove-redundant-character branch from a280db9 to 9138dbc Compare February 3, 2020 18:08
rstoyanchev pushed a commit that referenced this pull request Feb 3, 2020
rstoyanchev added a commit that referenced this pull request Feb 3, 2020
rstoyanchev added a commit that referenced this pull request Feb 3, 2020
As per the Javadoc of ConcurrentHashMap its computeIfAbsent
implementation is atomic and hence already synchronized internally,
so we can remove the surrounding synchronization block.

See gh-24470
@hyeonisism hyeonisism deleted the Remove-redundant-character branch February 9, 2020 14:47
sbrannen added a commit to sbrannen/spring-framework that referenced this pull request Oct 1, 2021
PR spring-projectsgh-24470 introduced a regression for Android users by no longer
escaping closing curly braces in regular expressions.

This commit therefore partially reverts the changes made in 273812f
for closing curly braces (`}`).

Closes gh27467
sbrannen added a commit to sbrannen/spring-framework that referenced this pull request Oct 1, 2021
PR spring-projectsgh-24470 introduced a regression for Android users by no longer
escaping closing curly braces in regular expressions.

This commit therefore partially reverts the changes made in 273812f
for closing curly braces (`}`).

Closes gh27467
@sbrannen sbrannen mentioned this pull request Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants