-
-
Notifications
You must be signed in to change notification settings - Fork 402
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
[Merged by Bors] - Implement ResolveLocale helper #2036
[Merged by Bors] - Implement ResolveLocale helper #2036
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2036 +/- ##
==========================================
+ Coverage 44.16% 44.31% +0.14%
==========================================
Files 212 212
Lines 18812 18912 +100
==========================================
+ Hits 8309 8380 +71
- Misses 10503 10532 +29
Continue to review full report at Codecov.
|
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.
Thanks for the contribution.
I commented on some things but I have some more general comments here.
Instead of working with String
s, it would probably be better to use JsString
s. We plan to switch to UTF-16 strings and these would need to be converted too.
There are some data structures in this code that the spec defines as Records
. For example I commented on the return value of ResolveLocale
. These are currently either JsValue
s or JsObject
s. I think it would be much better to have them as distinct struct types.
Also available_locales
and requested_locales
are JsValue
s but you convert them to objects and then use them as array-like objects. The spec specifies to use a List
, so it would be probably better to use a Vec<SomeType>
instead.
bf78acf
to
9d3cc79
Compare
@raskad do I need to change '&str' to '&JsString' as well? |
In theory I think yes. But think we can probably fix that later. Currently the most correct thing we could do is to use But I think this is probably not a top priority for this right now. Let's first get some working code, get some 262 tests to pass and then we can go from there ;) |
9d3cc79
to
f776bf4
Compare
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.
Just got my one comment left. Really nice work. Thank you for taking this on!
f776bf4
to
7f99dee
Compare
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.
Thank you, really nice work! I'm really exited for the first intl test to pass ;)
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.
Looks very good, just some documentation and Debug
derives missing, along with some coding standards improvements.
7f99dee
to
526d785
Compare
ec3e836
to
46b1b32
Compare
46b1b32
to
6a923dc
Compare
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.
Looks good to me! :)
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.
It looks great now!
bors r+ |
<!--- Thank you for contributing to Boa! Please fill out the template below, and remove or add any information as you feel neccesary. ---> This Pull Request implements ResolveLocale abstract method. It is required for further InitializeDateTimeFormat development. It changes the following: - Adds several helpers to operate with locale extensions - Adds DefaultLocale placeholder - Implements BestAvailableLocale and locale matchers - Implements UnicodeExtensionsComponents - Introduces testing
Pull request successfully merged into main. Build succeeded: |
<!--- Thank you for contributing to Boa! Please fill out the template below, and remove or add any information as you feel neccesary. ---> This Pull Request implements ResolveLocale abstract method. It is required for further InitializeDateTimeFormat development. It changes the following: - Adds several helpers to operate with locale extensions - Adds DefaultLocale placeholder - Implements BestAvailableLocale and locale matchers - Implements UnicodeExtensionsComponents - Introduces testing
This Pull Request implements ResolveLocale abstract method. It is required for further InitializeDateTimeFormat development.
It changes the following: