Status

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: stas, Assigned: stas)

Tracking

(Blocks 3 bugs)

Details

Attachments

(1 attachment)

Assignee

Description

3 years ago
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
Assignee

Updated

3 years ago
Assignee

Updated

3 years ago
Depends on: 1270146
Assignee

Updated

3 years ago
Depends on: 1288682
Assignee

Updated

3 years ago
Depends on: 1288684
Assignee

Updated

3 years ago
Depends on: 1288685
Assignee

Comment 1

3 years ago
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-
Assignee

Comment 3

3 years ago
(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.
Assignee

Updated

3 years ago
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.
Assignee

Comment 6

3 years ago
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
Last Resolved: 3 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.