search

ankidroid / Anki-Android

AnkiDroid: Anki flashcards on Android. Your secret trick to achieve superhuman information retention.
GNU General Public License v3.0
8.39k stars 2.18k forks source link

Lint: no Locale.ROOT in text user see #8917

Open Arthur-Milchior opened 3 years ago

Arthur-Milchior commented 3 years ago

It would be nice to add a lint rules that ensure that all text that user see is formatted with the local specified by Ressources. A recent PR added Locale.ROOT in a text that user could actually see, which should not occur.

It seems impossible to really define correctly in which case a text is user facing and in which case a text is internal. So maybe it should be a warning everything and suppress warning in acceptable uses. There are 26 uses right now

ShridharGoel commented 3 years ago

I would like to work on this.

mikehardy commented 3 years ago

No. There are cases the rule makes no sense, like the one Arthur is actually talking about. I don't like this rule.

mikehardy commented 3 years ago

Ah wait - having had a cup of coffee (I just woke up) - even formatting nothing but a single number is important because periods and commas are used differently etc. I forget that sometimes with my US-centric brain.

ShridharGoel commented 3 years ago

Ah wait - having had a cup of coffee (I just woke up) - even formatting nothing but a single number is important because periods and commas are used differently etc. I forget that sometimes with my US-centric brain.

I think this comment is meant for #8915.

mikehardy commented 3 years ago

That comment was locale-specific, as a code rule, it's in the right spot. #8915 is to me more about formatting (spacing) and how to implement that locally so that you don't have to wait so much for CI to tell you that you missed something.

Arthur-Milchior commented 3 years ago

@mikehardy I am really sorry, but I have no idea what you are suggesting. What is the path you want AnkiDroid to take?

  1. We don't automate anything and just try to catch errors in each PR
  2. we automate something, but something different than what I was asking for?
  3. we follow my suggestion?

This made me realize that my suggestion is actually not appropriate. I do not know what is appropriate but my suggestion would not have catched the original error until you added ROOT to it, which is clearly not what I want either. So I guess that what I really would have needed is to ensure that every single text shown to user either come from a ressource or is localized

mikehardy commented 3 years ago

3 - we follow your suggestion

mikehardy commented 3 years ago

Oh, and 3a. There was a lint rule that I followed while de-linting that was the proximate cause to this error, so it would catch it if we turned on both. I forget what the lint error was exactly but if you look at the code in the first de-lint transform it was this:

             browser.mActionBarTitle.setText(Integer.toString(browser.checkedCardCount()));

So whatever lint rule is triggered by doing setText on a text view in that style - turn that lint on and it will force you to use Locale's for transformation, then this lint check will force non-ROOT by default. There are very rare instances where you do want Locale.ROOT (I've needed it before and we may have one or more in AnkiDroid actually...) but then by default we will catch the rest and we can put in a suppression in for just the rare cases

github-actions[bot] commented 3 years ago

Hello 👋, this issue has been opened for more than 2 months with no activity on it. If the issue is still here, please keep in mind that we need community support and help to fix it! Just comment something like still searching for solutions and if you found one, please open a pull request! You have 7 days until this gets closed automatically

Arthur-Milchior commented 3 years ago

Are you stil on it @ShridharGoel (I know there are other priorities. But might be nice to let other know if it's available)

ShridharGoel commented 3 years ago

I'm not working on this. Anyone who might be interested can pick it.

github-actions[bot] commented 2 years ago

Hello 👋, this issue has been opened for more than 2 months with no activity on it. If the issue is still here, please keep in mind that we need community support and help to fix it! Just comment something like still searching for solutions and if you found one, please open a pull request! You have 7 days until this gets closed automatically

mikehardy commented 2 years ago

I think this is actually a reasonable first issue, reopening and tagging as such

vinaydeepkaur commented 1 year ago

Greetings, can I take it up?

mikehardy commented 1 year ago

Sure thing! Cheers

vinaydeepkaur commented 1 year ago

Can you please guide me what I need to do in this issue? I have learnt what lint rules are and how to implement them. I could locate the lint-rules directory in the code base. I am assuming I have to create a kotlin file in the lint directory and write a Detector class but I dont know what it has to detect. Am I thinking in the right direction? Can you please elaborate on what I should do or redirect me to some resources?

Thanks!

muhib-siddique commented 1 year ago

i want to work in this issue please asign me this issue

dae commented 1 year ago

@mikehardy what's the status of this?

mikehardy commented 1 year ago

Still needs an implementation and it would be useful as these are user-visible problems.

Solution is (based on above comments):