Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
i18n: upgrade to latest icu formatter #13834
i18n: upgrade to latest icu formatter #13834
Changes from 5 commits
504e62a
0b88142
7dd7d01
08bc98b
91c2243
c1a1197
24a384a
3c4b9bf
cf1b91c
5a7d4b7
7d643fd
080aea0
a609bb8
81b2330
bd14a91
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't exist any more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the error messages definitely seem less useful (at least looking at the test changes). Is there any kind of equivalent to replace this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a "location" field with begin/end markers, could use that to point to the specific characters that are problematic.
Can't promise fancy ascii pointers like you'd see elsewhere though :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For lhl validation purposes, It's possible testing that the actual substrings were valid lhlMessages may have been intentional?
option.value.elements
was available in the old version ofintl-messageformat-parser
as well. I think this was checking that it would be TC compatible, not justintl-messageformat
compatibleThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it the same? I think that part of the grammar is pretty recursive (as in, if this lib can parse it into a value, and that field is equivalent in structure to the root field, then the raw string should be valid as a string we'd send to TC?). I'll try to inspect the original PR and see if it was brought up.
FWIW, you can see the recursive test case failing if you comment this line out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily the same. For instance what's checked right above here: the ICU docs recommend that there be no content outside of plurals, but TC requires that there not be. Unfortunately everyone uses their own subset of the ICU/message format syntax.
The original PR doesn't list it's reasoning :( However, the fact that I wrote the "Each option value must also be a valid lhlMessage" comment above it to me means either there was a known mismatch, or the invariant is just what that comment describes and we should just check it directly rather than hoping there are no corner cases where they differ. Since this is in
collect-strings
, not in a lighthouse run or the report, seems reasonable?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The option value is really an array of messages. if one of the messages is a plural, then the validate function checks that it is the only entry (array length is 1 === no content outside of it). It seems like we're checking that correctly, and there's no failing test to say we aren't.
I do preferred this structured approach to reparsing the options... but we could do both just in case?