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

New value type: Variant List #102

Closed
stasm opened this issue Apr 27, 2018 · 7 comments
Closed

New value type: Variant List #102

stasm opened this issue Apr 27, 2018 · 7 comments
Labels

Comments

@stasm
Copy link
Contributor

stasm commented Apr 27, 2018

We currently allow Select Expressions to have a null selector. This is mostly useful in Terms to define facets of the term's value:

-brand-name =
    {
       *[nominative] Firefox
        [genitive] Firefox's
    }

The grammar defines the above syntax as:

Term
    id: Identifier
        name: "-brand-name"
    value: Pattern
        elements:
            - Placeable
                expression: SelectExpression
                    expression: null
                    variants: […]

Variants defined this way can be retrieved inside of Placeable with the -term[variant name] syntax. The runtime checks if the term's value is a pattern with a single element: a placeable which is a selector-less select expression. If the conditions are met, the variant is retrieved successfully.

The fact that we need this complex runtime check indicates to me that we're abusing Patterns for a logic-less (no selector) store for variants.

I'd like to make the design of Variant List more explicit by making them a new value type. Grammar-wise, values of Messages, Terms and Variants could either be Patterns or Variant Lists. Value List would not be expressions. Static analysis tools can then very easily tell when Variant Lists are mis-used: they shouldn't appear in Message at all, and they should only be nested inside of other Variant Lists.

The FTL snippet above would parse as follows:

Term
    id: Identifier
        name: "-brand-name"
    value: VariantList
        variants: […]
@stasm
Copy link
Contributor Author

stasm commented Apr 27, 2018

There still remains an inconsistency: a ValueList is not an expression so it cannot be inside of a Placeable.

# This wouldn't be allowed.
key =
    Foo Bar {
       *[name] Variant
    } Baz

But because they would be legal as values of other variants, this would be OK:

# This is OK.
key =
    Foo Bar { $num -> 
       *[other] {
           *[name] Variant
        }
    } Baz

To solve this, we'd need to make the variants in VariantList be a different grammar production than the variants in SelectExpression. Only variants in VariantList would allow nested VariantLists as their values.

I'm not sure if such distinction is necessary nor if it's worth the effort and the increase of complexity. From the use-cases point of view, a simple Message VariantList and SelectExpression VariantList CSS-like queries should be enough to warn against misuse.

@Pike
Copy link
Contributor

Pike commented Apr 27, 2018

There are aspects here that are not about if we do it or not, but where. Is it Syntax or Semantics that are statically checked.

I'd like to see both variants (pun intended), and see which of the two is easier to learn. We'll need to document both, one will be solely in the syntax, the other will be split between syntax and semantics.

@zbraniecki
Copy link
Collaborator

I like this proposal. I see a potential in the VariantList in the future so I'm happy to see it being formalized.

@stasm stasm added this to the Syntax 0.6 milestone May 18, 2018
@stasm stasm removed this from the Syntax 0.6 milestone May 23, 2018
@Pike
Copy link
Contributor

Pike commented May 25, 2018

Taking a question about intent here instead of the PR:

@stasm , I see the semantically correct usage of Variants in Terms, but when it comes down to Messages it seems weird.

I've filed #130 for private terms, which would be a way for a localization to hack around the lack of a Term. But I also don't like the idea of making a work-around that much work to use that in a context like:

A developer uses a message reference, but the string as is doesn't work as is in a particular locale. Say, because literal quoting isn't a thing in that language. To work around it, the locale would either drop the message ref, or add variants to the message. Or add a private term as proposed in #130.

Maybe this is more of a documentation problem than anything else?

@stasm
Copy link
Contributor Author

stasm commented May 25, 2018

I think we talked about forbidding VariantList in Messages all together. I implemented #129 based on the Syntax Semantics wiki page, but it didn't have any rules for Messages drafted. Thanks for the reminder. I now edited it with a proposal which I hope reflects what we talked about. I'll work on implementing these rules in #129 on Monday and I'll re-request review. It would help to land #128 first to avoid conflicts.

@stasm stasm removed the needs: PR label May 25, 2018
@stasm
Copy link
Contributor Author

stasm commented May 28, 2018

@Pike On Friday we talked about allowing VariantLists as values of Messages to support the use-case you mentioned two comments above, in #102 (comment). Should this extend to attributes of messages? Arguably, it would support the same use-case. However, in case of attributes, it would require a new syntax, message.attr[variant] which is a non-trivial change to the grammar. No member expressions currently allow other member expressions to be the parent. They only allow simple reference expressions in this position.

Because of this added complexity, I suggest we move this entire feature (VariantLists as values of Messages) out of Syntax 0.6. I'll file a new issue if you agree.

@Pike
Copy link
Contributor

Pike commented May 28, 2018

sgtm.

@stasm stasm added syntax and removed syntax labels Oct 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants