-
Notifications
You must be signed in to change notification settings - Fork 185
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
Locid extensions tofromstr #4934
Locid extensions tofromstr #4934
Conversation
Self::try_from_bytes(source.as_bytes()) | ||
} | ||
} | ||
|
||
writeable::impl_display_with_writeable!(Unicode); | ||
|
||
impl writeable::Writeable for Unicode { |
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 seems somewhat risky to change the Writeable
to not include the u-
as it has been doing since 1.0. It's easy for me to imagine code that depended on that behavior.
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 think we haven't thought it through sufficiently and I consider 2.0 a chance to normalize that. Stringification of a struct doesn't contain the extension key of the struct.
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, we can start landing 2.0 changes now. However, I'm not fully convinced. In the case of Other especially, it seems like printing out the extension key is very much the job of the extension struct. But, if you want to make this change (impl Writeable does not include u
), please do so consistently across Private, Other, and Transform.
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.
That's fair. I'm convinced by the compatibility with Other. I normalized the behavior across all extension types to require extension key for parsing and print it in response
b46e464
to
441426c
Compare
Self::try_from_bytes(source.as_bytes()) | ||
} | ||
} | ||
|
||
writeable::impl_display_with_writeable!(Unicode); | ||
|
||
impl writeable::Writeable for Unicode { |
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, we can start landing 2.0 changes now. However, I'm not fully convinced. In the case of Other especially, it seems like printing out the extension key is very much the job of the extension struct. But, if you want to make this change (impl Writeable does not include u
), please do so consistently across Private, Other, and Transform.
441426c
to
8a5f8d5
Compare
8a5f8d5
to
615825f
Compare
fb72fdb
to
4f0f713
Compare
4f0f713
to
cc45a10
Compare
Second part of #1833.
This one cleans up from/to str for locid extensions. This prepares ground for reuse of Subtag in Value API which will be introduced in the next PR.