-
Notifications
You must be signed in to change notification settings - Fork 390
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
Implement direct conversion functions #1227
base: master
Are you sure you want to change the base?
Conversation
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 like it 💖
It seems like a natural next step for the conversion functions you added in #588.
@@ -136,6 +136,12 @@ | |||
"BaseUnits": { | |||
"M": "Ounce" | |||
}, | |||
"Conversions": [ |
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.
DirectConversions
maybe, to be more explicit that this is an optional thing?
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.
@angularsen I think I would actually make it mandatory. I may take in stages, but maybe the first stage is the convert the current FromUnitToBaseFunc/FromBaseToUnitFunc to the Conversions?
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.
Oh gotcha, yes I suppose we could refactor the existing definitions to this.
The upside is more explicit naming, the old naming is kinda confusing. Also gathering all conversion functions, both direct and via base, in one place might be better.
The downside is that it is maybe not as obvious that the conversions need to go via a "base" unit, but we could maybe fail codegen if any unit has missing conversions from/to base unit?
Implementing direct conversion functions would help reduce error. Currently, to go from ounces to pounds, we go:
ounces -> kilograms -> pounds
This shows an example of going directly from ounces -> pounds.
Long term, we could move all FromUnitToBaseFunc/FromBaseToUnitFunc into this conversions array.
Just a quick example to show what's on my mind.