Enforce user-facing locale checks via lint; fixes #8917#18096
Enforce user-facing locale checks via lint; fixes #8917#18096mikehardy merged 1 commit intoankidroid:mainfrom
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. |
Arthur-Milchior
left a comment
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
1090bf5 to
8dcb5cc
Compare
| class LocaleRootDetectorTest { | ||
| @Language("JAVA") | ||
| private val invalidCode = | ||
| """ |
There was a problem hiding this comment.
Please remove the empty spaces
|
Hi there @spoisseroux! This is the OpenCollective Notice for PRs merged from 2025-03-01 through 2025-03-31 If you are interested in compensation for this work, the process with details is here: https://github.com/ankidroid/Anki-Android/wiki/OpenCollective-Payment-Process#how-to-get-paid We only post one comment per person per month to avoid spamming you, regardless of the number of PRs merged, but this note applies to all PRs merged for this month Please understand that our monthly budget is never guaranteed to cover all claims - the cap on payments-per-person may be lower, but we try to make our process as fair and transparent as possible, we just need your understanding. Thanks! |
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