Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add interchange data model description + JSON Schema definition #393
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
Add interchange data model description + JSON Schema definition #393
Changes from all commits
6bdc19a
4802d9b
655be51
a83ccb3
53c2aef
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, the ICU4J
Mf2DataModel
class has this method to get the message's variants:This is less convenient to implement in ICU4C than it is to return a list of
Variant
s, but I'm trying to do it anyway for the sake of parity with ICU4J.I like what you have here (
Variant[]
) better because it's easier to implement in C++, where ICU4C doesn't enjoy the benefits of Java's polymorphicOrderedMap
class. But also because while the list and map representations are isomorphic, I think it's more appealing to have an API that returns a list and let users build their own on top of it that does some kind of optimization for more efficient pattern-matching than it is to return a map when maybe efficient lookup isn't always necessary.Whether it ends up being a list or a map, mostly I just wanted to highlight that the ICU4J and ICU4C implementations should match what's defined here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A
FunctionRef
can also have aLiteral
or aVariableRef
as an argument, and in fact, due to how our syntax is designed, I'd argue that the function's argument is more important than the function. (E.g. it comes first in the syntax.)I'd like to suggest an alternative way to structure our expressions, to more closely map to our syntax:
Let's special-case argument-less functions rather than function-less operands.
Instead of
Literal | VariableRef | FunctionRef
here andoperand?: Literal | VariableRef
insideFunctionRef
, we can do:FWIW, this is how I implemented expressions in stasm/message2:
https://github.com/stasm/message2/blob/4abf43f2023b6e20d8ee1d462684d0741ece791b/syntax/ast.ts#L44-L70
(Not blocking this PR on this, but I'd like to discuss this change as a follow-up.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will be happy to discuss this further in a follow-on issue or PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed #436 to continue this.