Closed
Bug 1451450
Opened 6 years ago
Closed 6 years ago
[FTL] Incorrect warning when a term's variant is referenced in a string
Categories
(L20n :: Python Library, defect, P2)
L20n
Python Library
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: flod, Assigned: Pike)
References
Details
Attachments
(1 file, 1 obsolete file)
See this changeset https://hg.mozilla.org/l10n-central/it/rev/850a9445f26b compare-locales is reporting a warning, while the term is there, just with an additional variant selector WARNING: Missing message reference: -fxaccount-brand-name at line 390, column 1 for sync-signedout-account-title https://l10n.mozilla.org/dashboard/compare?run=952831
Assignee | ||
Comment 1•6 years ago
|
||
https://hg.mozilla.org/l10n/compare-locales/file/cdd7bc8c4232/compare_locales/checks.py#l471 needs to be extended to not just check for MessageReference, but also VariantExpression.id.name
Assignee | ||
Comment 2•6 years ago
|
||
Taking. There's a bit of a fudge in the checks for MessageReferences: If I add a "refer to { -term }" to a localized string, I get a warning. I can see how that's probably a good idea for Message references, but for Terms, I'm torn? Also, should we warn if the localization uses a message reference when en-US doesn't? I'm leaning towards yes here. Stas, flod?
Assignee: nobody → l10n
Flags: needinfo?(stas)
Flags: needinfo?(francesco.lodolo)
Priority: -- → P2
Assignee | ||
Comment 3•6 years ago
|
||
Piling on top: Stas, should VariantExpression actually subclass MessageReference instead of Expression? And should an AttributeExpression be handled as a l10n_id.attr reference?
Reporter | ||
Comment 4•6 years ago
|
||
(In reply to Axel Hecht [:Pike] from comment #2) > Taking. > > There's a bit of a fudge in the checks for MessageReferences: > > If I add a "refer to { -term }" to a localized string, I get a warning. I > can see how that's probably a good idea for Message references, but for > Terms, I'm torn? Can you explain this differently? Right now it sounds exactly like the next case. > Also, should we warn if the localization uses a message reference when en-US > doesn't? I'm leaning towards yes here. This can still lead to unexpected results, can't it? I add a term, but that's not available in the context. If that's the case, I think a warning is useful.
Flags: needinfo?(francesco.lodolo)
Assignee | ||
Comment 5•6 years ago
|
||
We're not running checks on term existence yet. Right now, the idea is that if it's in en-US, it should also be in the locale. Which right now is true, as we haven't added private terms yet. Maybe that's the point where we add different call signatures? ... rambling ....
Comment 6•6 years ago
|
||
(In reply to Axel Hecht [:Pike] from comment #3) > Piling on top: Stas, should VariantExpression actually subclass > MessageReference instead of Expression? That would be a good idea if it wasn't for the Dynamic References proposal (https://github.com/projectfluent/fluent/issues/80). We'll need to support VariantExpressions and AttributeExpressions on both MessageReferences and ExternalArguments: -term[varname] $object[varname] -term.attr $object.attr Which I think is best solved by adding another level of nesting to the AST, unfortunately. Right now, -term[varname] parses as: { "type": "VariantExpression", "id": { "type": "Identifier", "name": "-term" }, "key": { "type": "VariantName", "name": "name" } } In order to support both MessageReferences and ExternalArguments and to be able to serialize them, I think it should rather parse as: { "type": "VariantExpression", "of": { "type": "MessageReference", "id": { "type": "Identifier", "name": "-term" } }, "key": { "type": "VariantName", "name": "name" } } This is best visualized with the spans of $object[varname]: $object[varname] +----+ Identifier +-----+ ExternalArgument +------+ VariantName +--------------+ VariantExpression Note that this actually fixes the compare-locales check because the MessageReference node is now present in the AST, again.
Flags: needinfo?(stas)
Assignee | ||
Comment 7•6 years ago
|
||
This is actually best to fix in python-fluent, attaching a PR. I'll add testcases to compare-locales afterwards, too.
Attachment #8966537 -
Flags: review?(stas)
Assignee | ||
Updated•6 years ago
|
Component: compare-locales → Python Library
Product: Localization Infrastructure and Tools → L20n
Assignee | ||
Comment 8•6 years ago
|
||
https://github.com/projectfluent/python-fluent/commit/caa1efa3fdd8b94dce576a1a9b39511b90cba328 fixed this on the fluent.syntax side. We'll need a release of that to update compare-locales.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Attachment #8966537 -
Flags: review?(stas) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8966970 -
Attachment is obsolete: true
Attachment #8966970 -
Flags: review?(francesco.lodolo)
You need to log in
before you can comment on or make changes to this bug.
Description
•