Implement InitializeDateTimeFormat #2072
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2072 +/- ##
==========================================
+ Coverage 43.81% 44.43% +0.61%
==========================================
Files 217 217
Lines 19560 19978 +418
==========================================
+ Hits 8571 8877 +306
- Misses 10989 11101 +112
Continue to review full report at Codecov.
|
VM implementation
Fixed tests (36): |
|
@raskad, thanks for the update. I fixed these panicking tests locally, but I don't know how to gather that VM implementation report. My current method is to run failing script directly via Also, |
You are right, we should describe how to gather this report locally. git checkout main
cargo run --release --bin boa_tester -- run -vv -o results/main
git checkout <your-branch>
cargo run --release --bin boa_tester -- run -vv -o results/<your-branch>
cargo run --release --bin boa_tester -- compare results/mainlatest.json results/<your-branch>/latest.jsonYou may want to redirect the output of the I will review the PR in a bit ;) |
|
@NorbertGarfield Do note, however, that if you're running |
NorbertGarfield
force-pushed
the
garfield/init-date-time
branch
2 times, most recently
from
cb12567 to
a6fbb8a
Compare
NorbertGarfield
force-pushed
the
garfield/init-date-time
branch
4 times, most recently
from
0dca742 to
6e5f442
Compare
There was a problem hiding this comment.
Hi.
First of all, this is so exciting for us. We're building ICU4X particularly to target ECMA-402 needs well and so far we've mostly been testing it in context of SpiderMonkey use in Firefox.
Your work on boa+ecma-402 is a perfect fit and I'd love us to collaborate closer to supply your needs.
For this particular PR, I left some drive-by comments, and my overarching feedback, as a maintainer of locid component is - you should never have to work with strings. Any place where you substr or look for -u or anything like that feels like either icu_locid should provide you that, or you're doing something wrong. Generally speaking I strongly believe the whole implementation should always operate on a Locale type and only stringify/parse on output/input.
If something is missing or the API we have is suboptimal, let's fix it. It's a perfect timing as we're warming up to 1.0 stability but still have wiggle room left.
|
@zbraniecki Thank you for your review! As you can see, our On another note, we had a small problem with If we translate this to a high level operation, the spec requires removing the last word from the locale. Unfortunately, If that's not possible, then a way to deconstruct the internals of each extension would be good, because that would allow us to do something like: |
|
Uuh, that's really unfortunate. This seems to want you to operate on a stringified version of Locale, which i think we should avoid. There are other issues with it (like, "sr-Cyrl" should not be shortened to "sr"), but also, it makes the unreasonable attempt to cut out subtags from extensions. I think this operation should instead operate on I don't think we should be adding APIs to handle treating Locale as a string and make semantic operations based on positions of subtags. I think this is a change we should apply to ECMA-402 spec. CC @sffc for validation and we can file an issue there if he doesn't point out that I am missing something obvious. |
|
The spec text only requires that behavior be consistent with what would happen if you did the string operations on the locale string. You can implement it differently, so long as the result is the same. In this case, BCP47 guarantees that variant always comes after region which always comes after script which always comes after language, and you can build that assumption into your code. |
|
I don't think Language Identifiers are the problem per se; I can easily manipulate |
Support both '-u-' and 'u-' extension patterns.
NorbertGarfield
force-pushed
the
garfield/init-date-time
branch
from
6e5f442 to
617c5f4
Compare
I claim with the authority given to me by the ultimate powers (Shane in this case) that you should, for all purposes, treat this as a LanguageIdentifier. It just that ICU4X is the first time we provide API surface to the spec that identifies LI as a separate thing from Locale. I hereby recommend:
I'd particularly appreciate if you did (2) as I know it's easy to stop on (1) and move on :) Thank you! |
|
@zbraniecki I opened an issue in the spec and in the meantime we'll follow your suggestions and treat the input as a Language Identifier. Thank you for all your assistance! |
Improve test semantics Fetch default timezone from the system
There was a problem hiding this comment.
I think the main problem with the approach this PR takes is the many possible ways to lose correctness because of the types used in several definitions throughout the implementation.
For example, the DateTimeFormat type is defined as:
pub struct DateTimeFormat {
initialized_date_time_format: bool,
locale: JsString,
calendar: JsValue,
numbering_system: JsValue,
time_zone: JsString,
weekday: JsValue,
era: JsValue,
year: JsValue,
month: JsValue,
day: JsValue,
day_period: JsString,
hour: JsValue,
minute: JsValue,
second: JsValue,
fractional_second_digits: JsString,
time_zone_name: JsValue,
hour_cycle: JsValue,
pattern: JsString,
bound_format: JsString,
date_style: JsValue,
time_style: JsValue,
}So, what would happen if someone creates an instance of that type as follows?
pub struct DateTimeFormat {
initialized_date_time_format: true,
locale: "en-US".into(),
calendar: "gregori".into() // Notice the typo here,
numbering_system: "arab".into(),
...
}
We can't really tell what would happen in this case without scanning the whole module, and that's a loss for the maintainability of the code.
In the next days I'll implement an API that conflates the requirements for each Intl spec object into a collection of traits and types that preserves the correctness of our code.
|
you may want to track my work on icu_preferences which are targeting this problem space - unicode-org/icu4x#1833 I had a design brainstorm session with Shane last week and will push new revision soon, but I think it may simplify your logic quite a bit. |
|
There's a new Intl API in place for implementing Intl services more easily. This is waiting on the author to decide if they want to rebase on that or if we want to do it from scratch. |
|
@jedel1043, sorry for the late response. I'll try to implement it from scratch. I'm closing this PR. |
This Pull Request partially fixes #1562.
It changes the following: