messageformat / parser

A PEG.js parser for ICU MessageFormat strings
https://messageformat.github.io/
MIT License
7 stars 5 forks source link

Allow MessageFormat content in function parameters + replace `strictNumberSign` option with broader `strict` #8

Closed eemeli closed 6 years ago

eemeli commented 6 years ago

This is part of a possible solution for messageformat/messageformat#201, and in brief it allows for any formatted content in the optional argStyleText of a custom formatter function. To be clear, this is outside the official spec, not supported by other parsers, and it does theoretically break input that is valid according to the spec:

In argStyleText, every single ASCII apostrophe begins and ends quoted literal text, and unquoted {curly braces} must occur in matched pairs.

This change would not mean that any input that was previously ok with this library would not be ok after the change, as the parameter chars could not previously include an unquoted }.

This change would require a major-version update for messageformat-parser, but probably not for messageformat.

SlexAxton commented 6 years ago

Initially I’m not a fan of breaking from the spec, but I think maybe I’m ok with it as long as we mark it as non-compliant clearly in the docs or something.

Code looks good.

eemeli commented 6 years ago

I actually think that the spec is a bit broken on this, as it effectively says that the parameter string should be parsed as if it was a normal formattable string, but that it can't contain any actual formatting. It feels like the spec here just documents the implementation, rather than solving anything in the general case.

@SlexAxton, thoughts on adding a strictFunctionParams or similar flag for this? Not sure if it'd be worth it.

SlexAxton commented 6 years ago

As long as fully compliant stuff keeps working 100% doesn’t seem worth it. On Tue, Aug 14, 2018 at 4:55 PM Eemeli Aro notifications@github.com wrote:

I actually think that the spec is a bit broken on this, as it effectively says that the parameter string should be parsed as if it was a normal formattable string, but that it can't contain any actual formatting. It feels like the spec here just documents the implementation, rather than solving anything in the general case.

@SlexAxton https://github.com/SlexAxton, thoughts on adding a strictFunctionParams or similar flag for this? Not sure if it'd be worth it.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/messageformat/parser/pull/8#issuecomment-413029035, or mute the thread https://github.com/notifications/unsubscribe-auth/AAF5KqDCQvu1BMHHTbCThF7S5bVluiZUks5uQ0dRgaJpZM4Vu7xb .

eemeli commented 6 years ago

Unfortunately, this is related to messageformat/messageformat#201, where @johanblumenberg found input that is valid according to the spec, but which fails for messageformat. So we're arguably not 100% spec-compliant even now, and while this PR would make us not break on input that looks like it should work, that behaviour would not quite be following the spec.

SlexAxton commented 6 years ago

Seems like the better direction to be wrong :) On Tue, Aug 14, 2018 at 6:55 PM Eemeli Aro notifications@github.com wrote:

Unfortunately, this is related to messageformat/messageformat#201 https://github.com/messageformat/messageformat/issues/201, where @johanblumenberg https://github.com/johanblumenberg found input that is valid according to the spec, but which fails for messageformat. So we're arguably not 100% spec-compliant even now, and while this PR would make us not break on input that looks like it should work, that behaviour would not quite be following the spec.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/messageformat/parser/pull/8#issuecomment-413052340, or mute the thread https://github.com/notifications/unsubscribe-auth/AAF5KqjTpiYzso4s9uNFjvicsLW37y7Gks5uQ2N6gaJpZM4Vu7xb .

eemeli commented 6 years ago

Updated, now with a single strict option that should follow the spec more closely . This subsumes the strictNumberSign option we had before, and fixes prior non-compliance with functions: argType needs to be one of the preset ones, and the optional parameters are parsed according to spec.

SlexAxton commented 6 years ago

:+1: On Sat, Aug 25, 2018 at 8:49 PM Eemeli Aro notifications@github.com wrote:

Updated, now with a single strict option that should follow the spec more closely . This subsumes the strictNumberSign option we had before, and fixes prior non-compliance with functions: argType needs to be one of the preset ones, and the optional parameters are parsed according to spec.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/messageformat/parser/pull/8#issuecomment-415963865, or mute the thread https://github.com/notifications/unsubscribe-auth/AAF5KiAPkwCj6UtotFyz_3R9bjJOImXwks5uUTnWgaJpZM4Vu7xb .