-
-
Notifications
You must be signed in to change notification settings - Fork 11
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 MessageFormat2 support with ICU4J executor #175
Conversation
Thanks for your work on this. I look forward to integrating MF2! |
b11be59
to
9066446
Compare
testgen/testdata_gen.py
Outdated
|
||
# Utility functions | ||
def computeMaxDigitsForCount(count): | ||
return math.ceil(math.log10(count + 1)) | ||
|
||
|
||
def readFile(filename, version=''): | ||
def readFile(filename, version='', filetype='txt'): |
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.
How does Python allow people to attach doc strings for methods? Can you add a source doc string to document filetype
as having special behavior when you provide a value in the set of ['json']
?
7b4c9f4
to
1bb65d5
Compare
FYI, the current failure in the end-to-end job in CI is related to a configuration mistake in the Dart intl4x library that has been fixed and should roll out soon. It's obviously unrelated to our work, so ignore it for now. |
"value": 1.2 | ||
} | ||
], | ||
"verify": "=1.2" |
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 predict this test expected value is not correct and will need changing, once we get the test data hooked up to the executor. Plural selection happens on the "formatted" number (more precisely: the formatted-to-parts result, which comes after rounding is performed, notation changes applied, units are added, etc.), not the input number. I would expect the result to be other
.
By contrast, the previous test case's expected value seems fine, since 1.2 :integer
-> 1
(formatted) -> ONE
plural category (selection on formatted value).
"value": 1.2 | ||
} | ||
], | ||
"verify": "=1.2" |
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.
Ditto, but double problem: formatting should return 1
. The matching ought not to happen on the localized formatted string, but even if it did, the resulting string would be =1,2
, right? Also, does our syntax support ,
in a number literal? It doesn't seem so.
"description": "", | ||
"tests": [ | ||
] | ||
} |
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.
is this file proper JSON? the object that begins on line 1 ends on line 6.
beyond that, this file and some others that follow don't specify the default locale, FYI.
…er literal operand
@mradbourne @sven-oly Okay, I think this PR is ready to merge. I think I have it working locally. Here are some notes and caveats:
|
Ah, I forgot that properly viewing the output locally requires running a webserver via We have a graph now. Even though there are no passing tests, the information is useful in surfacing the disparities between implementation and spec design. Surfacing that type information so that people can act on it is what we're trying to achieve, so I think this is good to merge. We can leave the followup of fixing for a separate task. |
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.
OK, let's try this!
No description provided.