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)

defect

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
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
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
Piling on top: Stas, should VariantExpression actually subclass MessageReference instead of Expression?

And should an AttributeExpression be handled as a l10n_id.attr reference?
(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)
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 ....
(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)
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)
Component: compare-locales → Python Library
Product: Localization Infrastructure and Tools → L20n
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
Blocks: 1453277
Attachment #8966537 - Flags: review?(stas) → review+
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.

Attachment

General

Created:
Updated:
Size: