Add As/ToUnit conversions to UnitSystem #622
Conversation
|
Fat fingers + copy paste. A recipe for disaster in test naming 😆 |
Match the old descriptions and use singular unit instead of plural units.
| } | ||
|
|
||
| [Fact] | ||
| public void ToUnit_ImperialGivenSIUnitSystem_ReturnsSIValue() |
There was a problem hiding this comment.
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:
Assert.Equal(LengthUnit.SquareMeter, imperialArea.ToUnit(UnitSystem.SI).Unit)If so, maybe also rename the test to something like this ToUnit_ImperialQuantityGivenSIUnitSystem_ConvertsToSIUnit.
Same for other files below.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)) is true.
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.
| } | ||
| /// <summary> | ||
| /// Converts the quantity into a quantity which has units compatible with the given <see cref="UnitSystem"/>. |
There was a problem hiding this comment.
Grammar nazi 🎈 , but which should be preceded with a comma.
I also changed the descriptions a bit in IQuantity, so after you review those then match this xmldoc up with those and regen the files.
There was a problem hiding this comment.
Pinging this one, please review my xmldoc revisions in IQuantity, and make this consistent with that, then regen.
There was a problem hiding this comment.
Looks good to me. I just did an inheritdoc to keep things cleaner.
Adding AreAllBaseUnitsDefined method
Doc update
|
Yup! Sorry, I got busy with my daughter and didn't get to finish. Let me take a look now. |
Inherit doc
|
Ok nice! |
angularsen
changed the title
| } | ||
|
|
||
| [Fact] | ||
| public void ToUnit_ImperialGivenSIUnitSystem_ReturnsSIValue() |
There was a problem hiding this comment.
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)) is true.
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.
|
Oh yea. #629 to fix. |
No description provided.