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

ImplicitDefaultLocale rule should not check for deprecated functions #6343

Closed
marschwar opened this issue Jul 31, 2023 · 10 comments · Fixed by #6548
Closed

ImplicitDefaultLocale rule should not check for deprecated functions #6343

marschwar opened this issue Jul 31, 2023 · 10 comments · Fixed by #6548
Assignees
Labels
good first issue Issue that is easy to pickup for people that are new to the project help wanted rules

Comments

@marschwar
Copy link
Contributor

marschwar commented Jul 31, 2023

Expected Behavior of the rule

The ImplicitDefaultLocale rule should not check for usage of toUpperCase and toLowerCase.

Context

The extension functions String.toUpperCase and String.toLowerCase have been deprecated in favor of uppercase and lowercase. Both function by default use the Locale.ROOT and make the check obsolete

@marschwar marschwar added rules help wanted good first issue Issue that is easy to pickup for people that are new to the project labels Jul 31, 2023
@atulgpt
Copy link
Contributor

atulgpt commented Jul 31, 2023

Hi, @marschwar should this rule be updated to handle uppercase(and similarly lowercase) asking as these new methods already handle locale info correctly by using Locale.ROOT which is not system dependent.

Or is an action item of this issue is to remove this rule?

@marschwar
Copy link
Contributor Author

TBH I wondered the same thing and hoped this would start a discussion. I am not entirely sure if the problem this rule addresses is still present with Locale.Root or not.

@atulgpt
Copy link
Contributor

atulgpt commented Aug 1, 2023

if you see the doc of Root locale at https://docs.oracle.com/javase/8/docs/api/java/util/Locale.html#ROOT then you will find below doc(emphasis mine)

Useful constant for the root locale. The root locale is the locale whose language, country, and variant are empty ("") strings. This is regarded as the base locale of all locales, and is used as the language/country neutral locale for the locale sensitive operations.

So to me, it seems that Locale.ROOT basically handles the original issue(my understanding of the original issue is that when using the old methods which use default JVM locale sometimes we end up getting some different capital or small letters based on the locale)

@marschwar
Copy link
Contributor Author

I agree. We could either close this issue then or remove the toUpperCase and toLowerCase handling from the rule. As it has been deprecated since 1.5 I would opt for removal. What do you think?

@atulgpt
Copy link
Contributor

atulgpt commented Aug 11, 2023

I agree. We could either close this issue then or remove the toUpperCase and toLowerCase handling from the rule. As it has been deprecated since 1.5 I would opt for removal. What do you think?

I agree as this is now inherently handled by the newer methods. While the older methods are deprecated(and it's also been long since those methods are deprecated)

@marschwar marschwar changed the title ImplicitDefaultLocale rule should check uppercase instead of toUpperCase ImplicitDefaultLocale rule should not check for deprecated functions Aug 11, 2023
@marschwar
Copy link
Contributor Author

I have updated the issue title and description.

@BraisGabin
Copy link
Member

Which is the use case? You don't want to get the same issue twice? One by the compiler and the other by detekt?

@marschwar
Copy link
Contributor Author

marschwar commented Aug 13, 2023

It is not about the deprecation warning. This rule currently checks, that those two deprecated methods are used correctly.

How have we handled this in the past? Should we really check if deprecated methods are used correctly?

@BraisGabin
Copy link
Member

I don't think we handled this in the past. TL;DR: I agree, we can remove the support for those two functions.

@Gosunet
Copy link
Contributor

Gosunet commented Oct 11, 2023

Hello, I can propose a PR to do that if you like?

If I understand correctly I just need to delete the check?

Gosunet added a commit to Gosunet/detekt that referenced this issue Oct 11, 2023
toUpperCase and toLowerCase are deprecated, the ImplicitDefaultLocale should not check usage for them anymore

Closes detekt#6343
Gosunet added a commit to Gosunet/detekt that referenced this issue Oct 15, 2023
toUpperCase and toLowerCase are deprecated, the ImplicitDefaultLocale should not check usage for them anymore

Closes detekt#6343
Gosunet added a commit to Gosunet/detekt that referenced this issue Oct 21, 2023
toUpperCase and toLowerCase are deprecated, the ImplicitDefaultLocale should not check usage for them anymore

Closes detekt#6343
BraisGabin pushed a commit to Gosunet/detekt that referenced this issue Nov 9, 2023
toUpperCase and toLowerCase are deprecated, the ImplicitDefaultLocale should not check usage for them anymore

Closes detekt#6343
BraisGabin pushed a commit that referenced this issue Nov 9, 2023
…6548)

toUpperCase and toLowerCase are deprecated, the ImplicitDefaultLocale should not check usage for them anymore

Closes #6343
cortinico pushed a commit that referenced this issue Nov 25, 2023
…6548)

toUpperCase and toLowerCase are deprecated, the ImplicitDefaultLocale should not check usage for them anymore

Closes #6343
mgroth0 pushed a commit to mgroth0/detekt that referenced this issue Feb 11, 2024
…etekt#6548)

toUpperCase and toLowerCase are deprecated, the ImplicitDefaultLocale should not check usage for them anymore

Closes detekt#6343
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issue that is easy to pickup for people that are new to the project help wanted rules
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants