Closed Bug 1288639 Opened 8 years ago Closed 8 years ago

Implement Intl.MessageContext

Categories

(L20n :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: stas, Assigned: stas)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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
Depends on: 1270146
Depends on: 1288682
Depends on: 1288684
Depends on: 1288685
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/
Attachment #8775605 - Flags: review?(l10n)
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.
Attachment #8775605 - Flags: review?(l10n) → review-
(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.
Attachment #8775605 - Flags: review?(gandalf)
Attachment #8775605 - Flags: review?(gandalf) → review+
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.
Not closing this yet so that I can address Pike's feedback from comment 2 with a follow-up landing.
Assignee: nobody → stas
Status: NEW → ASSIGNED
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.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
No longer depends on: 1270146, 1288682, 1288684, 1288685
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: