MINOR: Make regular expressions into static #5168
MINOR: Make regular expressions into static #5168hachikuji merged 1 commit intoapache:trunkfrom KoenDG:staticpattern3
Conversation
|
ping @ijuma sorry for dropping this for so long. Merged in trunk and pushed, all builds passing, should be good. |
hachikuji
left a comment
hachikuji
left a comment
There was a problem hiding this comment.
Thanks for the patch. Just had a minor nit.
There was a problem hiding this comment.
nit: comments like this seem redundant. Can we just remove them?
There was a problem hiding this comment.
If that's what you want, I'll remove them. My reason for adding was that it wasn't immediately obvious to me what the regex did. So I went to regex101.com and it explained the details, which I then added. It might be redundant if you're more fluent at regex than me, but it wasn't for me.
There was a problem hiding this comment.
Also, it's more for other people than for me. I want the people to come after me to be able to read this and find use out of it, instead of having to google it. I already did that, I might as well save the people after me the trouble.
There was a problem hiding this comment.
@hachikuji Just to clarify, I'm waiting on your confirmation that you still want it gone after reading my reasoning.
There was a problem hiding this comment.
@KoenDG I agree with @hachikuji. Name of the constant should be explanatory enough (and it usually is). I don't see the need to explain usage further.
andrasbeni
left a comment
andrasbeni
left a comment
There was a problem hiding this comment.
@KoenDG Thanks for you contribution. I have a few questions and remarks.
There was a problem hiding this comment.
@KoenDG I agree with @hachikuji. Name of the constant should be explanatory enough (and it usually is). I don't see the need to explain usage further.
There was a problem hiding this comment.
Is it necessary to add SPLIT_ and REPLACE_ to the names of patterns? The operation performed with the pattern is not part of it. In theory one pattern could be used to both split and replace.
There was a problem hiding this comment.
This one matches a double quote, not a backslash and a double quote.
There was a problem hiding this comment.
This one matches a single backslash, not a double backslash. Also, I think you don't need to abbreviate 'backslash'.
There was a problem hiding this comment.
This nested invocation is much harder to read than the original version. I believe storing the result of the first replace in a local variable would make it clearer.
There was a problem hiding this comment.
In OAuthBearerUnsecuredLoginCallbackHandler.java you use Matcher.quoteReplacement. Why do you do it differently here?
There was a problem hiding this comment.
Looks like an oversight. The first thing the source code of Matcher.quoteReplacement does is this:
if ((s.indexOf('\\') == -1) && (s.indexOf('$') == -1))
return s;
So anything with a \ gets processed and therefor needs it.
That being said, one should not rely on knowledge of source code to assume behavior. I'll add them everywhere to be consistent.
There was a problem hiding this comment.
Scratch that, now that I'm looking at the actual code, it because of the difference between replace and replaceAll in String.java.
replace will do Matcher.quoteReplacelemt, replaceAll doesn't. And the javadoc for replaceAll warns the user of this and suggests using it beforehand if desired.
Knowing that, I'll make it so that the behavior is that of the function that originally got called.
EDIT: And there's more. replace interprets the regex as LITERAL. replaceAll doesn't.
…ing them every single time.
|
PR updated, as per comments. |
|
+1. LGTM. |
hachikuji
left a comment
hachikuji
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the patch and also the additional reviews.
|
@hachikuji Thanks. Is this good for merge now? I don't have an option to do that. |
|
Sorry, got sidetracked when I was about to merge. Will do so shortly |
|
Thanks you for your time. |
…ache#5168) Reviewers: Andras Beni <andrasbeni@cloudera.com>, Sriharsha Chintalapani <sriharsha@apache.org>, Jason Gustafson <jason@confluent.io>
4 participants
@ijuma I made a new PR after several rebases and merges of trunk. It's the same as this: #5042 except only 1 commit.
This should improve performance, as in-place means that the regex has to be compiled again each time the code path is traversed. Which is not performant.
A nice writeup on this topic can be found here: https://softwareengineering.stackexchange.com/questions/216320/java-regex-patterns-compile-time-constants-or-instance-members
And it's also recommended in Joshua Bloch's "Effective Java 3rd Edition" (Item 6: Avoid creating unnecessary objects)
Committer Checklist (excluded from commit message)