Skip to content
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

Introduce StringCaseLocaleUsage check #3813

Conversation

rickie
Copy link
Contributor

@rickie rickie commented Mar 9, 2023

Fixes #3809.

Copy link
Collaborator

@cushon cushon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

.build();
}

private static int getClosingParenPosition(MethodInvocationTree tree, VisitorState state) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, did you see examples like .toUpperCase /* Comment with parens: (). */() in practice?

It's nice to handle this correctly, but I would normally have just done something like replace(getEndPosition(tree.getMethodSelect()), getEndPosition(tree), "(Locale.ROOT)")

Copy link
Contributor Author

@rickie rickie Mar 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, no we didn't encounter a lot of problems, we created this after our initial implementation (that was based on a String#replaceFirst) which wasn't correct enough.

It's indeed a bit more complex than what you are proposing. We can do 3 things:
(1) Extract this functionality to a utility class and re-use this setup, which might make it worth that it is slightly more complex.
(2) Keep it as-is.
(3) Use the method you are proposing.

Let me know what you prefer 😄.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest creating a helper in SuggestedFixes, but I'm not sure how general it is without a way to choose where the argument is inserted, and I don't have a great ideas for the API. Maybe something like addArgumentToMethod with a parameter for the insertion index?

How about (2) for now, and if you feel like it you can add a TODO to consider promoting it to SuggestedFixes later :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deal, let's go for (2), I'll add a TODO.

@@ -0,0 +1,73 @@
package com.google.errorprone.bugpatterns;
Copy link
Collaborator

@cushon cushon Mar 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need the copyright headers, I can also add them during the import if that's easier.

/*
 * Copyright 2023 The Error Prone Authors.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

('The Error Prone Authors' refers to https://github.com/google/error-prone/blob/master/AUTHORS, you're welcome to add yourself there also. There's some more detail about that in https://opensource.google/documentation/reference/releasing/authors)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will push a commit with the copyright headers.

you're welcome to add yourself there also.

That'd be cool! Added that in the commit.

I can't reach the website with more details though, it asked me to do a sign in with a "@google.com" email, so it's probably an internal thing?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, there's an external mirror with the same content, I just pasted the wrong link. That should have been https://opensource.google/documentation/reference/releasing/authors

@rickie
Copy link
Contributor Author

rickie commented Mar 10, 2023

Thanks for the quick responses. Added the commits 😄.

copybara-service bot pushed a commit that referenced this pull request Mar 10, 2023
Fixes #3809.

Fixes #3813

FUTURE_COPYBARA_INTEGRATE_REVIEW=#3813 from PicnicSupermarket:rossendrijver/StringCaseLocaleUsage 59abfd4
PiperOrigin-RevId: 515721685
@Stephan202 Stephan202 deleted the rossendrijver/StringCaseLocaleUsage branch May 11, 2023 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Discourage APIs locale-dependent APIs like String.to{Lower,Upper}Case
2 participants