Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throw fewer runtime errors for # #156

Merged
merged 4 commits into from
Sep 1, 2016
Merged

Conversation

eemeli
Copy link
Member

@eemeli eemeli commented Aug 1, 2016

This relaxes the default runtime treatment of #, and expands the meaning of MessageFormat#setStrictNumberSign(true) to return to the previous, stricter treatment.

Before, we had this:

var mf = new MessageFormat('en');
var msg = '{X, plural, one{the thing} other{# things}}';
mf.compile(msg)({ X: 3 });
  // '3 things'
mf.compile(msg)({ X: 'cool' });
  // Error: 'cool' from argument 'X' isn't a number.

With this PR, we get this:

var mf = new MessageFormat('en');
var msg = '{X, plural, one{the thing} other{# things}}';
mf.compile(msg)({ X: 3 });
  // '3 things'
mf.compile(msg)({ X: 'cool' });
  // 'cool things'
mf.compile(msg)({});
  // 'undefined things'
mf.setStrictNumberSign(true);
mf.compile(msg)({ X: 'cool' });
  // Error: Argument `X` has non-numerical value "cool".

To get that to work, I needed to make the runtime depend on the MessageFormat class, which was easier to do by separating Runtime to be more clearly its own class, defined in lib/runtime.js.

@SlexAxton
Copy link
Member

I don't think anyone is going to turn on setStrictNumber in real life, but I'm roughly fine with that. I'd be more comfortable if we added that option to the docs before we merged, but am cool either way.

@eemeli
Copy link
Member Author

eemeli commented Sep 1, 2016

That may well be, but I at least am happier with us supporting standard-compliance at least optionally. At least the API documentation will get updated from the JSDoc comments, but the public site could indeed do with an update to match the 1.0 release.

I'll merge this and see if I could get that finally done as well.

@eemeli eemeli merged commit dd9c8d5 into messageformat:master Sep 1, 2016
@eemeli eemeli deleted the lax-number branch September 1, 2016 19:00
@cletusw
Copy link
Member

cletusw commented Sep 1, 2016

@eemeli Feel free to ping me on your docs changes if you'd like a second pair of eyes on the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants