Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Further reduce usages of
StringUtils
#9044Further reduce usages of
StringUtils
#9044Changes from all commits
f31afb2
aa5e0af
aeb5060
6434727
b817155
e7ee9bb
0fa773e
e942cc1
c3f10bd
99ccb39
12a0efc
744f3db
9e9e714
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 1438 in core/src/main/java/hudson/PluginManager.java
ci.jenkins.io / Code Coverage
Partially covered line
Check warning on line 1439 in core/src/main/java/hudson/PluginManager.java
ci.jenkins.io / Code Coverage
Partially covered line
Check warning on line 1440 in core/src/main/java/hudson/PluginManager.java
ci.jenkins.io / Code Coverage
Partially covered line
Check warning on line 1444 in core/src/main/java/hudson/PluginManager.java
ci.jenkins.io / Code Coverage
Partially covered line
Check warning on line 501 in core/src/main/java/hudson/PluginWrapper.java
ci.jenkins.io / Code Coverage
Partially covered line
Check warning on line 133 in core/src/main/java/hudson/cli/CLIAction.java
ci.jenkins.io / Code Coverage
Partially covered line
Check warning on line 137 in core/src/main/java/hudson/cli/CLIAction.java
ci.jenkins.io / Code Coverage
Partially covered line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The extra
o
variable seems like an unnecessary layer of indirection for the reader. Why not just name itexpectedOrigin
to begin with and remove this line.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expectedOrigin
is a local variable used in lambda, it must be afinal
oreffectively final
.Of course we can set
o
as another name for readability, but I think it is not very necessary as a temporary variable.Check warning on line 175 in core/src/main/java/hudson/model/AutoCompletionCandidates.java
ci.jenkins.io / Code Coverage
Partially covered line
Check warning on line 1302 in core/src/main/java/hudson/model/UpdateSite.java
ci.jenkins.io / Code Coverage
Partially covered line
Check warning on line 1303 in core/src/main/java/hudson/model/UpdateSite.java
ci.jenkins.io / Code Coverage
Not covered line
Check warning on line 64 in core/src/main/java/hudson/search/FixedSet.java
ci.jenkins.io / Code Coverage
Partially covered line
Check warning on line 674 in core/src/main/java/hudson/security/SecurityRealm.java
ci.jenkins.io / Code Coverage
Partially covered line
Check warning on line 675 in core/src/main/java/hudson/security/SecurityRealm.java
ci.jenkins.io / Code Coverage
Not covered line
Check warning on line 232 in core/src/main/java/hudson/slaves/JNLPLauncher.java
ci.jenkins.io / Code Coverage
Partially covered line
Check warning on line 245 in core/src/main/java/hudson/slaves/JNLPLauncher.java
ci.jenkins.io / Code Coverage
Partially covered line
Check warning on line 491 in core/src/main/java/jenkins/install/SetupWizard.java
ci.jenkins.io / Code Coverage
Not covered lines
Check warning on line 67 in core/src/main/java/jenkins/security/BasicHeaderProcessor.java
ci.jenkins.io / Code Coverage
Partially covered line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it is unlikely to matter in practice, this is not a sound replacement. If a request was sent with
then a server running in Turkish locale (e.g., #9066, though here it is the server not client locale which matters) would fail this clause because
is false.
In general, you always want to pass the
Locale
argument (almost alwaysLocale.ROOT
) totoLowerCase
/toUpperCase
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BobDu Is it worth filing a follow-up PR to address the above feedback? Or do we think the code is fine as-is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree always add
Locale.ROOT
arg totoLowerCase
method is a best practices.Already make a new PR #9162