Refactor measure unit parser by using fixed unit ids #6683
Conversation
# Description - Removed unused `power` and `si_prefix` modules from the measure module. - Updated tests in area, length, mass, and volume categories to directly use `MeasureUnitParser::try_from_str` instead of creating a default parser instance. - Introduced new parser functionality for handling unit identifiers and SI prefixes, improving the overall structure and readability of the code.
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback. |
younies
requested review from
a team,
Manishearth,
robertbastian and
sffc
as code owners
| pub struct MeasureUnitParser; | ||
|
|
||
| impl MeasureUnitParser { |
There was a problem hiding this comment.
the type is no longer needed
| pub struct MeasureUnitParser; | |
| impl MeasureUnitParser { | |
| impl MeasureUnit { |
There was a problem hiding this comment.
I prefer to have the MeasureUnitParser separate from MeasureUnit. Users will rarely need to use the parser, and in my opinion, separating it (along with the data) from MeasureUnit results in better organization.
There was a problem hiding this comment.
I don't think it makes sense to have a parser type that doesn't have any state and is not constructible. We don't do this for any other types that have a try_from_str method.
There was a problem hiding this comment.
by the way, it will be constructable in the future when will we allow the users to add their own units :)
| pub struct MeasureUnitParser; | ||
|
|
||
| impl MeasureUnitParser { |
There was a problem hiding this comment.
I don't think it makes sense to have a parser type that doesn't have any state and is not constructible. We don't do this for any other types that have a try_from_str method.
robertbastian
left a comment
robertbastian
left a comment
There was a problem hiding this comment.
there are still open comments from my last reviews
sffc
left a comment
sffc
left a comment
There was a problem hiding this comment.
Seems like a step in the right direction
3 participants
Description
powerandsi_prefixmodules from the measure module to parser module.MeasureUnitParserwithout depending on an external data.MeasureUnitParser::try_from_strinstead of creating a default parser instance.NOTE
We need to add a future possibility for adding new units, but for now we can have a fixed place for unit names and ids.