-
Notifications
You must be signed in to change notification settings - Fork 176
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
Impl Serde for PackedPatternsV1 with packed-like human form #5620
Conversation
I'll add @robertbastian as an optional reviewer since you were reviewer on the previous PR, or you can defer to @Manishearth. |
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 there any context for this? I don't understand why you are spending 3h on a pretty serde impl on an unused type that does not affect testdata.
🎉 All dependencies have been resolved ! |
It will replace the thing currently called |
I spent 3h on a pretty serde impl because my first attempt added API surface we didn't want to add, so I had to write a manual serde impl. It has been important for the datetime patterns to be human-readable in JSON, and they wouldn't have been human-readable if I had used the default human-readable serialization of the machine struct. |
Can you link an issue? |
Caused by clash of #5620 and #5601 <!-- Thank you for your pull request to ICU4X! Reminder: try to use [Conventional Comments](https://conventionalcomments.org/) to make comments clearer. Please see https://github.com/unicode-org/icu4x/blob/main/CONTRIBUTING.md for general information on contributing to ICU4X. -->
It's done now. #5648 |
Replaces #5592
Depends on #5621
Depends on #5622
This PR does not export
impl Deserialize for PluralElements
, which was going to prevent #5592 from landing. However, if we ever add plural packed patterns, we're going to need something like that.(note on my coding velocity: this PR took me just over 3 hours to write, which I consider to be a lot of time for a serde impl. the previous PR took less than an hour.)