-
Notifications
You must be signed in to change notification settings - Fork 400
Add As/ToUnit conversions to UnitSystem #622
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
Changes from all commits
387565a
d9068e5
ed35fc0
036aae5
3454219
66c7dbe
1ca4282
420d118
01f5900
cfee3d6
406d590
e30a841
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
If I understand it right, this test would pass regardless if we pass it SI unit system or some other system. The quantity doesn't change when changing the unit.
Maybe the test should instead assert this:
If so, maybe also rename the test to something like this
ToUnit_ImperialQuantityGivenSIUnitSystem_ConvertsToSIUnit
.Same for other files below.
Uh oh!
There was an error while loading. Please reload this page.
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 will pass only for a unit system that has a base unit in meters. You could define one in feet and it would have a different quantity value and unit (whatever value in square feet). That conversion is handled by other tests, but I added it here regardless. I can change it for sure.
Uh oh!
There was an error while loading. Please reload this page.
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.
Wait, I forgot this one before merging.
Are you saying that this test will only pass because
ToUnit(UnitSystem.SI)
converts to the Meter unit?I think that is false, because the Area equality check converts to the default/base unit before comparing values.
As an example, I just tested this:
Length.FromCentimeters(100).Equals(Length.FromMeters(1))
istrue
.This is why I think this test does not actually test the
ToUnit
behavior at all - because it could have incorrectly converted to nautical mile unit and it would still equal the left hand side.