Lol wow, nice work. I might need a few days to go through this. That cool?

(and I'm sure you see tests are failing)

Taking a little time to review is absolutely fine. :)

Found the test-failure-generator. It turns out the node fs.write() call has a form I'm using in tests that was only introduced in node 0.12, and we're now failing with 0.8 and 0.10. Will fix.

Any thoughts yet, @SlexAxton?

My other thought:

The format-js guys technically pulled out our old peg parser and changed some things but otherwise were kind of very similar. I very seriously doubt we could use their peg as it is, but since there's is an external dependency, maybe if we forked it, made our necessary changes and used it as a dependency it'd increase the chances of our implementations at least converging at the AST level.

Does that sound crazy?

Regarding the plural/selectordinal keys, the spec is actually a bit weird on this, as it doesn't limit the keys, but does expect and require the input for the matching to be a number. Effectively, it's messageformat.js that's been incorrect here because we've not required the input to be a number. I checked a few of the other implementations, and it may just have been us that worked this way.

I have now made the generated error more descriptive, and added a method disablePluralKeyChecks() to return to the previous functionality. Another option would be to keep the checks, but console.warn instead of throwing an error.

While doing that, I also moved the make-plural require() statements back inside getPluralFunc() -- which also needed to get a bit more complex for the checks to be optional in the parser. This way, if you're bringing in plural functions yourself, make-plural is never called.

Regarding the parser, I presume you mean this? I did take a look at it while refactoring our parser, but ended up doing things a bit differently; the version I ended up with is IMHO much simpler. Changing the yahoo/intl-messageformat compiler doesn't look trivial, but I agree that there may be use cases for our parser to be used by itself. I'll have a try at extracting it from this repo, and see how it looks.

Given the scope of the changes in this PR, it might make sense to push out a v1.0.0-rc.1 prerelease version from it, and see what breaks in real use cases. Would that be ok?

Go for it. Looking solid.
On Sun, Apr 17, 2016 at 5:02 AM Eemeli Aro [email protected] wrote:

Regarding the plural/selectordinal keys, the spec is actually a bit weird
on this, as it doesn't limit the keys, but does expect and require the
input for the matching to be a number. Effectively, it's messageformat.js
that's been incorrect here because we've not required the input to be a
number. I checked a few of the other implementations, and it may just have
been us that worked this way.

I have now made the generated error more descriptive, and added a method
disablePluralKeyChecks() to return to the previous functionality. Another
option would be to keep the checks, but console.warn instead of throwing
an error.

While doing that, I also moved the make-plural require() statements back
inside getPluralFunc() -- which also needed to get a bit more complex for
the checks to be optional in the parser. This way, if you're bringing in
plural functions yourself, make-plural is never called.

Regarding the parser, I presume you mean this
http:///yahoo/intl-messageformat-parser/blob/master/src/parser.pegjs? I
did take a look at it while refactoring our parser, but ended up doing
things a bit differently; the version I ended up with
http:///eemeli/messageformat.js/blob/simpler-output/lib/parser.pegjs is
IMHO much simpler. Changing the yahoo/intl-messageformat compiler
http:///yahoo/intl-messageformat/blob/master/src/compiler.js doesn't
look trivial, but I agree that there may be use cases for our parser to be
used by itself. I'll have a try at extracting it from this repo, and see
how it looks.

Given the scope of the changes in this PR, it might make sense to push out
a v1.0.0-rc.1 prerelease version from it, and see what breaks in real use
cases. Would that be ok?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#138 (comment)

That was decently painless. I extracted the full history of edits to the parser, and constructed a new repo around that at messageformat/parser. That's now published to npm with a "latest" release of v1.0.0-rc.1, and can be installed with:

npm install messageformat-parser

I've subsequently removed the parser code from messageformat.js, messed around a bit, and got it released on bower and npm:

bower install messageformat#1.0.0-rc.2
npm install messageformat@prerelease

I think despite my fixes, the bower release may still be missing the main jsdoc style file. I'll look at it later. I've left this branch on my own account for now, as github won't let the base branch of an existing PR to be changed. There is, however, the github prerelease tag v1.0.0-rc.2 that bower uses.

@eemeli @SlexAxton Any ETA on this? Anecdotally, I've tried 1.0.0-rc.2 in a very large project and it seems to be working fine, but I'm hesitant to push it to production in case there are known issues that I've just missed.

I feel pretty confident that the RC2 is pretty close to what will be released. Usually it's docs and stuff that are holding back releases, the code seems to be pretty solid.

@SlexAxton @eemeli Sorry to keep pestering -- just take it as meaning somebody really appreciates your work :)

What's left to be done for the 1.0.0 release? Anything I can help with?

@cletusw, the main reason for slow progress is lack of time and focus on other projects. I finally got around to addressing the issues that had popped up with RC2, and pushed out RC3. We can probably re-release it as 1.0.0 shortly, unless any further issues come up with it.

I totally understand. Thanks again for the amazing work!

I merged the branch to SlexAxton/master, as I probably should've done a while back. This should help e.g. @cletusw base his octothorpe error PR properly.