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

Specify Locale when calling String#to{Lower,Upper}Case #336

Closed
1 of 2 tasks
rickie opened this issue Nov 5, 2022 · 6 comments · Fixed by #376
Closed
1 of 2 tasks

Specify Locale when calling String#to{Lower,Upper}Case #336

rickie opened this issue Nov 5, 2022 · 6 comments · Fixed by #376
Labels
Milestone

Comments

@rickie
Copy link
Member

rickie commented Nov 5, 2022

Problem

When calling String#to{Lower,Upper}Case without arguments, the system's Locale is used by default.
To prevent this we want our code to specify Locale.ROOT.
Therefore, we want to rewrite occurrences that invoke the aforementioned methods without Locale and introduce Locale.ROOT.

Description of the proposed new feature

  • Support a stylistic preference.
  • Avoid a common gotcha, or potential problem.

I would like to rewrite the following code:

"A".toLowerCase();
"B".toUpperCase();

to:

"A".toLowerCase(Locale.ROOT);
"B".toUpperCase(Locale.ROOT);

Considerations

Just a consideration; instead of Locale.ROOT one can also use other Locales. I assume we want to leave those as is, right?

Participation

This is a nice opportunity for contributing an Error Prone check 😄.

@rickie rickie added good first issue Good for newcomers new feature labels Nov 5, 2022
@Stephan202
Copy link
Member

Stephan202 commented Nov 5, 2022

Just a consideration; instead of Locale.ROOT one can also use other Locales. I assume we want to leave those as is, right?

Certainly!

This is a nice opportunity for contributing a Refaster rule 😄.

I'd rather see this implemented using a BugChecker which suggest the Locale.ROOT fix by default, but proposes Locale.getDefault() as a secondary fix. The latter is (a) behavior preserving, (b) may be more appropriate in certain contexts (such as desktop applications) and (c) might make for an easier migration in large code bases. (I.e.: first do the safe thing by using Locale.getDefault() everywhere, disallow new .to{Lower,Upper}Case() calls, then review and replace such usages if/when possible.

@rickie
Copy link
Member Author

rickie commented Nov 8, 2022

That makes sense, let's create a BugChecker for this then. I updated the description 👍🏻.

@mlrprananta
Copy link
Contributor

I'm looking to pick this one up, any pointers before I begin?

@rickie
Copy link
Member Author

rickie commented Nov 11, 2022

Yes @mlrprananta for sure.

I would say this will be quite similar to the FluxFlatMapUsage, RefasterAnyOfUsage and (a little more complex maybe:) the IdentityConversion checks.

On a first glance, it sounds like the BugChecker needs to implement the MethodInvocationTreeMatcher and use a custom Matcher (see the aforementioned checks for some examples) and match on the methods from this ticket.
Next up, there will be two SuggestedFixes, just as @Stephan202 describes here. The FluxFlatMapUsage also has multiple fixes, so there you can also see how we test that 😄.

Are these pointers helpful? If not, or if you have any questions, feel free to reach out 😉.

@mlrprananta
Copy link
Contributor

Opened a PR (#376), PTAL 🙂

@rickie rickie linked a pull request Dec 6, 2022 that will close this issue
@rickie
Copy link
Member Author

rickie commented Dec 6, 2022

Resolved in #376.

@rickie rickie closed this as completed Dec 6, 2022
@Stephan202 Stephan202 added this to the 0.6.0 milestone Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

3 participants