1288639 - Implement Intl.MessageContext

Status: RESOLVED FIXED

(L20n :: General, defect)

Description

[Staś Małolepszy :stas]

Intl.MessageContext is a proposal which builds on top of MessageFormat [1] and extends it to provide support for better interpolation, language-specific "traits", built-in types and functions and error handling.

It uses the FTL syntax described here [2].

The first implementation of Intl.MessageContext will be a JSM.



[1] http://wiki.ecmascript.org/doku.php?id=globalization:messageformatting
[2] https://github.com/stasm/l20n-syntax-experiments/tree/master/ftl

Comment 1

[Staś Małolepszy :stas]

Attachment: Bug 1288639 - Implement MessageContext,

This is a JSM polyfill for a future Intl.MessageContext proposal.

Review commit: https://reviewboard.mozilla.org/r/67770/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67770/

Comment 2

[Axel Hecht [:Pike]]

Comment on attachment 8775605 [details]
Bug 1288639 - Implement MessageContext,

https://reviewboard.mozilla.org/r/67770/#review64822

My comments are mostly about making the code readable, and closer to other jsms in toolkit/modules.

Just overall, this code needs comments. Which is also why I'm not looking closely yet, I'd prefer to check if the code does what the comments explain, than trying to figure out what the code does and if it does so correctly at the same time.

Also, I think that most jsms order their code in the opposite order, and I find that easier to digest.

You start with a top-level comment (this code does X).
Then the exports.
Then imports and lazys.
Then the definition of the exports.
Then the code that you need to implement that.

That way, you can read the code from top to bottom, and you don't have to jump to the end first, and then up.

This is a very high-level r-, and I kinda feel bad about it. But I also feel OK about it, 'cause I'm not requesting comments for the first time, and review sounds like the right stage to enforce that.

Comment 3

[Staś Małolepszy :stas]

(In reply to Axel Hecht [:Pike] from comment #2)
> Comment on attachment 8775605 [details]
> Bug 1288639 - Implement MessageContext,
> 
> https://reviewboard.mozilla.org/r/67770/#review64822

Thanks for taking a look, Pike.  I should have probably asked for f? at this point which would have possibly made this easier to r- :)

> My comments are mostly about making the code readable, and closer to other
> jsms in toolkit/modules.
> 
> Just overall, this code needs comments. Which is also why I'm not looking
> closely yet, I'd prefer to check if the code does what the comments explain,
> than trying to figure out what the code does and if it does so correctly at
> the same time.

Would you say that the lack of comments should block us from landing on larch?  We have a dedicated bug for that (bug 1288685) so that we can move forward with things which require MessageContext (like l20n-chrome-xul.js and the Menu bar localization) without blocking right now.

On a very deep level I think gandalf can give this a round of review without the comments for now.  Just to reiterate:  I definitely to document this code (with comments and with and RST doc).

> Also, I think that most jsms order their code in the opposite order, and I
> find that easier to digest.
> 
> You start with a top-level comment (this code does X).
> Then the exports.
> Then imports and lazys.
> Then the definition of the exports.
> Then the code that you need to implement that.

This file is built from a number of smaller files in l20n.js.  It will be very hard to enforce any specific ordering from rollup and I'd question the usefulness of such effort.

Comment 4

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

Comment on attachment 8775605 [details]
Bug 1288639 - Implement MessageContext,

https://reviewboard.mozilla.org/r/67770/#review64886

Yup, r+.

This is fairly trivial merge of already reviewed code from l20n.js repo. Parser, Resolver and a small Object to store entities and format them.

Comment 5

[Staś Małolepszy :stas]

https://hg.mozilla.org/projects/larch/rev/4e808f44b30e7182f5358f63bd89abb3b7c36cdd
Bug 1288639 - Implement MessageContext, r=gandalf

Comment 6

[Staś Małolepszy :stas]

Not closing this yet so that I can address Pike's feedback from comment 2 with a follow-up landing.

Comment 7

[Staś Małolepszy :stas]

https://hg.mozilla.org/projects/larch/rev/530dc1b58520ac87909eed9f22633712d5003830
Bug 1288639 - Add Intl.ListFormat and Intl.PluralRules stubs

Comment 8

[Axel Hecht [:Pike]]

I'll spin off the questions of making the code readable into a new bug.

Breaking dependencies on bugs 1270146, 1288682, 1288684, 1288685, as we're marking this FIXED, so they obviously didn't block us.