-
Notifications
You must be signed in to change notification settings - Fork 400
Add ReciprocalLength #954
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
Add ReciprocalLength #954
Conversation
…eract with ReciprocalLength quantity.
…h ReciprocalLength quantity.
…with ReciprocalLength quantity.
…erator overloads for Force and ForcePerLength quantities to interact with ReciprocalLength quantity.
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.
Thanks! It looks good, just some suggestions below:
"Units": [ | ||
{ | ||
"SingularName": "InverseMeter", | ||
"PluralName": "InverseMeter", |
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.
InverseMeters
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.
Fixed.
[InlineData(0.0, 0.0)] | ||
[InlineData(1.0, 1.0)] | ||
[InlineData(2.0, 0.5)] | ||
public static void InverseTest(double value, double expected) |
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 see you already copied this test to UnitsNet.Tests/CustomCode/ReciprocalLengthTests.cs
, so you can remove this.
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 thought this test was part of the LengthTests so an optical illusion happened for me :). Made this test a part of the LengthTests.
[InlineData(0.0, 0.0)] | ||
[InlineData(1.0, 1.0)] | ||
[InlineData(2.0, 0.5)] | ||
public static void InverseTest(double value, double expected) |
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.
Nitpick, but you could rename this InverseReturnsLength
.
|
||
protected override double InverseMeterInOneInverseMeter => 1; | ||
protected override double InverseCentimeterInOneInverseMeter => 1E-2; | ||
protected override double InverseMillimeterInOneInverseMeter => 1E-3; |
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.
protected override double InverseCentimeterInOneInverseMeter => 1E-2; | ||
protected override double InverseMillimeterInOneInverseMeter => 1E-3; | ||
|
||
protected override double InverseMileInOneInverseMeter => 1 / 0.000621371; |
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.
Please use constants looked up in an external reference, instead of calculations.
This is meant to serve as a double check that we got it right, since it is very easy to get it inverse and still pass all tests.
According to this converter, InverseMileInOneInverseMeter should be 1609.344
.
https://www.unitsconverters.com/en/Reciprocal-Length-Conversions/Measurement-1182
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.
You're right. I was not sure at the beginning and there is an inconsistency throughout the repo so I've picked the fraction way. Now fixed.
public partial struct ReciprocalLength | ||
{ | ||
/// <summary> | ||
/// Calculates the inverse or <see cref="Length"/> of this unit. |
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.
Nitpick, but ReciprocalLength
is a quantity and not a unit. And since we don't map from InverseYard to Yard, maybe we could describe it like this:
Returns the inverse quantity <see cref="Length"/> with the unit <see cref="LengthUnit.Meter />.
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.
Updated.
Great work! Nuget is on the way out. |
Added Reciprocal (Inverse) Length quantity and unit.