-
Notifications
You must be signed in to change notification settings - Fork 174
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
Split MeasureUnit
into MeasureUnitParser
and MeasureUnit
#4391
Conversation
MeasureUnitParser
🎉 All dependencies have been resolved ! |
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.
Good for experimental. Looking forward to when we start running it with data
impl MeasureUnit { | ||
impl<'data> MeasureUnitParser<'data> { | ||
/// Creates a new MeasureUnitParser from a ZeroTrie payload. | ||
pub fn new(zerotrie_payload: &'data ZeroTrie<ZeroVec<'data, u8>>) -> Self { |
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.
issue: the public API should construct this from the data struct, through a provider. this looks like a datagen-only API, so it should be behind the datagen feature
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.
let's discuss this API before submission in the ICU4X meeting today.
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.
On talking to Younies: the way to construct the Parser should be a method from the MeasureUnitConversionFactory which is what owns the data
We should:
- Temporarily mark this
cfg(feature = datagen)
(since it's still needed by datagen) - Rename it to
from_payload()
- Add a todo to revisit the public nature of the API
- (separate PR) make this constructable from a Factory
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.
done
Consider renaming the PR to "Split MeasureUnit into MeasureUnitParser and MeasureUnit". You're not really implementing anything here. |
MeasureUnitParser
MeasureUnitParser
and MeasureUnit
MeasureUnitParser
and MeasureUnit
MeasureUnit
into MeasureUnitParser
and MeasureUnit
No description provided.