Enforce user-facing locale checks via lint; fixes #8917 #18096
Conversation
|
First PR! 🚀 We sincerely appreciate that you have taken the time to propose a change to AnkiDroid! Please have patience with us as we are all volunteers - we will get to this as soon as possible. |
There was a problem hiding this comment.
Hi,
First, thanks so much for starting to contribute.
I'd really love some tests with that. You can look at DirectSystemTimeInstantiationTest to see how to test linters. Because, to be honest, I've a hard time reading Linter code, it's very specific and this is not something I read every day, while tests are very easy to verify.
Are you here for GSoC? If so, we also expect you to create a test, so that'd be nice too. If so, I'd like to add, as a mentor, that it'd be great if you also had a contribution that introduce a user facing change. It's always easier for me, as a mentor, to evaluate Android code; than code processing code
|
|
||
| private fun isLocaleRootUsage(node: UCallExpression): Boolean = | ||
| node.methodName?.let { methodName -> | ||
| (methodName == "format" || methodName == "getInstance" || methodName == "newInstance") && |
There was a problem hiding this comment.
I'm not a fan to be honest. I'm far from certain that this is exactly all of the function where Locale.ROOT should not be used.
I think we should just forbid it everywhere, and then enable it in the case where we find out it's really needed, as Mike suggested in the issue.
I'd be fine however if you explicitly exclude some functions where we know we really don't care about the locale.
I'm fine if you just suppress the Lint error on all usages of Locale.ROOT currently used in the code. Ideally I'd love a comment with it, but I'd understand if you lack context to explain the choice in some of the places.
There was a problem hiding this comment.
Yeap, I'm here from GSoC. I have a merged unit test here.
Could you recommend me a user-facing issue that would be good for a beginner? There weren't many not currently being worked on that have the Good First Issue label. I took this as I was recently learning about linters so thought it was a good fit, sorry about that.
I will update to forbid everywhere, and suppress of current uses of Locale.ROOT (most likely without the comments) and implement tests.
Thanks so much for the feedback!
Added lint rule to ensure proper locale usage in text formatting - Implemented custom `LocaleRootDetector` to flag `Locale.ROOT` in user-facing formatting methods (e.g. String.format, NumberFormat) - Enabled `DefaultLocale` check to require explicit Locale parameters - Fixed violations in AxisValueDisplay and other components Testing: - Ran `./gradlew lint` to verify new checks flag violations
Arthur-Milchior
force-pushed
the
fix-8917-locale-root-lint
branch
from
1090bf5 to
8dcb5cc
Compare
| class LocaleRootDetectorTest { | ||
| @Language("JAVA") | ||
| private val invalidCode = | ||
| """ |
There was a problem hiding this comment.
Please remove the empty spaces
Arthur-Milchior
added
the
Needs Second Approval
github-actions
bot
removed
the
Needs Second Approval
Purpose / Description
User-facing text formatting sometimes uses Locale.ROOT or lacks explicit locale, leading to incorrect number/date formats
Fixes
Approach
How Has This Been Tested?
./gradlew clean lint
Learning (optional, can help others)
Android custom lint rules
Used existing custom lint rules as a template
Checklist