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

CLDR-17857 add whole-locale warning for missing English name #3924

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

srl295
Copy link
Member

@srl295 srl295 commented Jul 31, 2024

  • warning, not error, otherwise we'd block builds!
  • new subtype missingEnglishName
  • also remove LogKnownIssues for CLDR-15663

CLDR-17857

  • This PR completes the ticket.

ALLOW_MANY_COMMITS=true

srl295 added 2 commits July 31, 2024 13:35
- no logKnown issue is needed, the test is already correct.
- English translation will not be needed until coverageLocales.txt is updated.
- warning, not error, otherwise we'd block builds!
- new subtype missingEnglishName
@srl295 srl295 self-assigned this Jul 31, 2024
@srl295
Copy link
Member Author

srl295 commented Jul 31, 2024

Example output:

lld [Ladin]	warning	▸null◂	〈null〉	【】	〈null〉	«»	【】	⁅missing english name⁆	❮Warning: Missing English translation for lld❯	…
::warning file=lld.xml,title=missing english name:: Warning: Missing English translation for lld

@AEApple
Copy link
Contributor

AEApple commented Jul 31, 2024

So will this just start failing when data is brought into vXML, or is there a way to get it to fail earlier in the cycle?

@srl295
Copy link
Member Author

srl295 commented Jul 31, 2024

So will this just start failing when data is brought into vXML, or is there a way to get it to fail earlier in the cycle?

Good question. Two parts to the answer:

  1. There's already a test. Once a locale reaches basic - and localeCoverage.txt is updated - tests will start failing as follows. These tests pass today, so there aren't any issues currently. This PR removes a logKnownIssues for locales below basic.
CLDR {
  LikelySubtagsTest {
    TestMissingInfoForLanguage {
      Error: (LikelySubtagsTest.java:345) Missing English translation for: zzz which is at basic
    } (0.268s) FAILED (1 failure(s))

This would fail after vxml import, and after updating the coverage levels. It's at that point that we would need to update English.

  1. Secondly, this PR adds a new 'whole locale warning' - which will show up in the survey tool (once we no longer are suppressing those) and show up as a warning - in the locale - that the English name is missing.

@macchiati
Copy link
Member

#2 does not appear to be the right answer, to me.

The TC is the body that wants to get the information that an English name for a code xxx is missing, not vetters in locale xxx.

@srl295
Copy link
Member Author

srl295 commented Jul 31, 2024

#2 does not appear to be the right answer, to me.

The TC is the body that wants to get the information that an English name for a code xxx is missing, not vetters in locale xxx.

CLDR-15663 is titled "Ask for English version of language name when collecting Core data". So to expand on #2, I would say that if there still isn't an English version of the name when this warning shows up, the vetter could file a ticket, but it would be up to the TC to actually implement the change to English - at TC's discretion. The "ask" is to provide an input - not a definitive data point.

@AEApple
Copy link
Contributor

AEApple commented Jul 31, 2024

Agreed, it seems like it would be nice if the error started showing up in en instead, and then we have en show up in the priority issues tracking?

@srl295
Copy link
Member Author

srl295 commented Jul 31, 2024

Agreed, it seems like it would be nice if the error started showing up in en instead, and then we have en show up in the priority issues tracking?

That would be another way to do it. However, en is readonly, does anyone check en for errors at runtime in the survey tool? Edit OK. I stand corrected, there are errors in en. Still not clear where they would show up tracking-wise, that's why I thought in the locale (as with 'missing plural rules') made some sense. As with missing plural rules, it's an issue that cannot be fixed within the survey tool, which blocks progress towards coverage goals.

Edit2 would both make sense? A warning on the English side and on the locale side linking to that English warning? Then it would be in front of people looking at the locale side.

@AEApple
Copy link
Contributor

AEApple commented Aug 1, 2024

We should just update priority issue tracking to include errors in en for future releases.

@macchiati
Copy link
Member

macchiati commented Aug 1, 2024 via email

@macchiati
Copy link
Member

macchiati commented Aug 1, 2024 via email

Copy link
Member

@macchiati macchiati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think this is the best approach, but let's discuss

@srl295
Copy link
Member Author

srl295 commented Aug 3, 2024 via email

@macchiati
Copy link
Member

macchiati commented Aug 3, 2024 via email

@srl295
Copy link
Member Author

srl295 commented Aug 3, 2024 via email

@srl295
Copy link
Member Author

srl295 commented Aug 6, 2024

I added a proposal to today's meeting agenda - will copy it back to the ticket after discussion.

@srl295 srl295 marked this pull request as draft August 6, 2024 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants