Implement MeasureUnit #4360
Merged
Implement MeasureUnit #4360
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
younies
commented
Nov 23, 2023
younies
commented
Nov 23, 2023
younies
requested review from
sffc,
robertbastian and
Manishearth
as code owners
Comment on lines
259
to
262
| let (num_part, den_part) = identifier | ||
| .split_once("-per-") | ||
| .or_else(|| identifier.split_once("per-")) | ||
| .unwrap_or((identifier, "")); |
There was a problem hiding this comment.
I realized we actually only need one search:
Suggested change
| let (num_part, den_part) = identifier | |
| .split_once("-per-") | |
| .or_else(|| identifier.split_once("per-")) | |
| .unwrap_or((identifier, "")); | |
| let (num_part, den_part) = identifier | |
| .split_once("per-") | |
| .map(|(n,d)| (n.strip_suffix("-").unwrap_or(n), d)) | |
| .unwrap_or((identifier, "")); |
There was a problem hiding this comment.
okay, done, but I feel we over complicated it
robertbastian
requested changes
Nov 30, 2023
Comment on lines
193
to
197
| if let Some(unit_id) = trie.get(part.as_bytes()) { | ||
| Some(unit_id) | ||
| } else { | ||
| None | ||
| } |
There was a problem hiding this comment.
Suggested change
| if let Some(unit_id) = trie.get(part.as_bytes()) { | |
| Some(unit_id) | |
| } else { | |
| None | |
| } | |
| trie.get(part.as_bytes()) |
or just inline
sffc
reviewed
Dec 1, 2023
| // TODO(#4369): split this struct to two structs: MeasureUnitParser for parsing the identifier and MeasureUnit to represent the unit. | ||
| pub struct MeasureUnit { | ||
| /// Contains the processed units. | ||
| pub contained_units: Vec<MeasureUnitItem>, |
There was a problem hiding this comment.
Suggestion: this should be either SmallVec (as in fixed_decimal) or ShortBoxSlice (as in icu_locid). The common case shouldn't need to allocate a Vec.
| /// Get the power of the unit. | ||
| /// NOTE: | ||
| /// if the power is found, the function will return (power, part without the power). | ||
| /// if the power is not found, the function will return (1, part). |
| /// NOTE: | ||
| /// if the power is found, the function will return (power, part without the power). | ||
| /// if the power is not found, the function will return (1, part). | ||
| fn get_power(part: &str) -> Option<i8> { |
There was a problem hiding this comment.
Nit (optional): I prefer free functions instead of static functions in Rust except for constructors.
There was a problem hiding this comment.
I was thinking about this too. I have moved them to si+prefix.rs and power.rs
| /// Get the SI prefix. | ||
| /// NOTE: | ||
| /// if the prefix is found, the function will return (power, base, part without the prefix). | ||
| /// if the prefix is not found, the function will return (0, Base::NotExist, part). |
robertbastian
approved these changes
Dec 6, 2023
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.
No description provided.