[Merged by Bors] - Redesign Intl API and implement some services #2478
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jedel1043
added
enhancement
jedel1043
requested review from
Razican,
jasonwilliams,
HalidOdat,
RageKnify,
raskad and
nekevss
|
Oops, forgot this was on my personal fork 😅. I'll copy-paste the CI results |
Codecov Report
@@ Coverage Diff @@
## main #2478 +/- ##
==========================================
- Coverage 52.99% 51.53% -1.47%
==========================================
Files 343 353 +10
Lines 34755 35615 +860
==========================================
- Hits 18419 18353 -66
- Misses 16336 17262 +926
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Test262 conformance changes
Fixed tests (516):Broken tests (6): |
Razican
reviewed
Dec 10, 2022
|
n.b. left a reply on a resolved comment, which won't be easy to find since it's hidden, see #2478 (comment) |
Y'all should be generating your own custom default data, not using icu_testdata. icu_testdata is for our tests, and also for people to be able to try ICU4X out without making a full data pipeline. An easy thing to do woudl be to run the ICU4X datagen tool with the locales you need and produce either postcard or baked (faster) data. Then check that in to this repo or a separate repo that gets submoduled, and regenerate periodically when you start adding more stuff. |
Noted 👍 |
|
Ok, I generated the locale data using the |
Manishearth
reviewed
Dec 15, 2022
| //! implementation. | ||
| //! | ||
| //! # Crate Overview | ||
| //! This crate exports the function [`blob`], which contains an extensive dataset of locale data to |
There was a problem hiding this comment.
suggestion: y'all might benefit from using the "baked" backend for datagen which generates a very efficient Rust crate.
There was a problem hiding this comment.
Thought about using that, but when I try to include!() the generated code, Rust-analyzer goes crazy and starts throwing errors on the whole workspace.
There was a problem hiding this comment.
Yeah it's a very large crate full of const data that rust-analyzer doesn't like.
The trick we use is include!(concat!(env!("CARGO_MANIFEST_DIR"), "/data/baked/mod.rs"));, and we have the generated code live in data instead of src
There was a problem hiding this comment.
Yeah, I used the exact same trick, but R-A insists on trying to analyze the generated file :(
There was a problem hiding this comment.
The weird thing is that when I open one of the generated files, R-A marks the // @generated with something like "file not included in analysis"
There was a problem hiding this comment.
we can make it a datagen option to do that so it's easier, if that works
there really needs to be a way to tell r-a to bugger off
There was a problem hiding this comment.
Yeah. Though, it'll also need to be accompanied with a way to declare the public exports of a file, or else R-A will complain about missing symbols.
There was a problem hiding this comment.
Tried to rename the included files with no success 🙁
We'll use binary blobs in the meantime, and I'll try to keep up with R-A to switch to baked data when the issue gets fixed. Are there any related issues I should subscribe to?
There was a problem hiding this comment.
We haven't filed any because so far it's worked for us. I think it might be because we use Cargo features.
Y'all should go ahead and file something and cc me.
There was a problem hiding this comment.
jedel1043
force-pushed
the
intl-api
branch
3 times, most recently
from
645a871
to
aeece51
Compare
|
Thanks to unicode-org/icu4x#2892, we've got a couple more passing tests after updating the dependencies. I've got some more PRs in the pipeline, such as using |
HalidOdat
approved these changes
Dec 26, 2022
|
Thank you for reviewing this in spite of the fact that we're still on holidays 😁 |
bors bot
pushed a commit
that referenced
this pull request
Dec 26, 2022
This Pull Request fixes/closes #1180. (I'll open a tracking issue for the progress) It changes the following: - Redesigns the internal API of Intl to (hopefully!) make it easier to implement a service. - Implements the `Intl.Locale` service. - Implements the `Intl.Collator` service. - Implements the `Intl.ListFormat` service. On the subject of the failing tests. Some of them are caused by missing locale data in the `icu_testdata` crate; we would need to regenerate that with the missing locales, or vendor a custom default data. On the other hand, there are some tests that are bugs from the ICU4X crate. The repo https://github.com/jedel1043/icu4x-test262 currently tracks the found bugs when running test262. I'll sync with the ICU4X team to try to fix those. cc @sffc
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
bors bot
pushed a commit
that referenced
this pull request
Jan 19, 2023
Follows from #2528, and should complement #2411 to implement the module import hooks. ~~Similarly to the Intl/ICU4X PR (#2478), this has a lot of trivial changes caused by the new lifetimes. I thought about passing the queue and the hooks by value, but it was very painful having to wrap everything with `Rc` in order to be accessible by the host. In contrast, `&dyn` can be easily provided by the host and has the advantage of not requiring additional allocations, with the downside of adding two more lifetimes to our `Context`, but I think it's worth.~~ I was able to unify all lifetimes into the shortest one of the three, making our API just like before! Changes: - Added a new `HostHooks` trait and a `&dyn HostHooks` field to `Context`. This allows hosts to implement the trait for their custom type, then pass it to the context. - Added a new `JobQueue` trait and a `&dyn JobQueue` field to our `Context`, allowing custom event loops and other fun things. - Added two simple implementations of `JobQueue`: `IdleJobQueue` which does nothing and `SimpleJobQueue` which runs all jobs until all successfully complete or until any of them throws an error. - Modified `boa_cli` to run all jobs until the queue is empty, even if a job returns `Err`. This also prints all errors to the user.
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This Pull Request fixes/closes #1180. (I'll open a tracking issue for the progress)
It changes the following:
Intl.Localeservice.Intl.Collatorservice.Intl.ListFormatservice.On the subject of the failing tests. Some of them are caused by missing locale data in the
icu_testdatacrate; we would need to regenerate that with the missing locales, or vendor a custom default data.On the other hand, there are some tests that are bugs from the ICU4X crate. The repo https://github.com/jedel1043/icu4x-test262 currently tracks the found bugs when running test262. I'll sync with the ICU4X team to try to fix those.
cc @sffc