1310148 - Create reference documentation for Builtins

Status: RESOLVED FIXED

(L20n :: Evangelism, defect)

Description

[Axel Hecht [:Pike]]

We don't seem to have a reference documentation for Builtins like LEN etc yet?

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: pr

Comment 2

[Axel Hecht [:Pike]]

Comment on attachment 8802751 [details] [review]
pr

I think we need to make the options explicit in our documentation. For one, I think our documentation should be self-contained, but also because the intl spec is really hard to decipher, if you're not acquainted with the nomenclature.

I'm not sure if we should keep the "In the future we might" part. I'm even wondering if that's a question for the binding part? For example:

  Default options for builtins can be passed in by the developer, if the API supports it. At this point, there's no API that does that, though.

I wonder if we should keep the PLURAL builtin, mostly because we don't implement it as documented. It's really just an alias for NUMBER, and I wonder if it's easier to explain why people should use the same options in the selector as in the placable.

PS: PLURAL is one of the things that made me file this bug when I tried to read the code of which builtins we have ;-)

Comment 3

[Staś Małolepszy :stas]

(In reply to Axel Hecht [:Pike] from comment #2)
> Comment on attachment 8802751 [details] [review]
> pr
> 
> I think we need to make the options explicit in our documentation. For one,
> I think our documentation should be self-contained, but also because the
> intl spec is really hard to decipher, if you're not acquainted with the
> nomenclature.

+1

> 
> I'm not sure if we should keep the "In the future we might" part. I'm even
> wondering if that's a question for the binding part? For example:
> 
>   Default options for builtins can be passed in by the developer, if the API
> supports it. At this point, there's no API that does that, though.

That's actually already supported by MessageContext: https://github.com/l20n/l20n.js/blob/3ba0b9ad8f20a9540eb7ef6c4bda4604af4677e1/src/intl/polyfill.js#L6-L7


> I wonder if we should keep the PLURAL builtin, mostly because we don't
> implement it as documented. It's really just an alias for NUMBER, and I
> wonder if it's easier to explain why people should use the same options in
> the selector as in the placable.

I think we should remove it.  I filed bug 1311662 to have that discussion there.  Zibi did a good job documenting the current state of the code.  Let's discuss changes to the code in separate bugs.

I also filed bug 1311666 in which I suggest we rename OS to PLATFORM.

Comment 4

[Staś Małolepszy :stas]

Comment on attachment 8802751 [details] [review]
pr

I left my feedback at https://github.com/l20n/l20n.js/pull/160#pullrequestreview-5051881.  I'd like to see another iteration.

Comment 5

[Staś Małolepszy :stas]

Removing the dependencies as they aren't really blocking this.

Comment 6

[Staś Małolepszy :stas]

Also note that the parser requires `:` for the keyword arguments, as well as quotes:

    today-is = Today is { DATETIME($date, weekday: "long") } 

https://github.com/l20n/l20n.js/blob/3ba0b9ad8f20a9540eb7ef6c4bda4604af4677e1/src/ftl/ast/parser.js#L503

We should probably change the EBNF grammar: https://github.com/l20n/spec/blob/126b422ae146cdb81eed38484eca8d732cec46a6/grammar.ebnf#L49

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8802751 [details] [review]
pr

carry over r+ from Stas on IRC