Skip to content
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 UsGallonsPerMile, ImperialGallonPerMile, MegaukGallonPerDay, MegausGallonPerDay #1225

Merged
merged 11 commits into from
Apr 1, 2023

Conversation

t03apt
Copy link
Contributor

@t03apt t03apt commented Mar 13, 2023

ImperialGallonPerMile
UsGallonPerMile
MegausGallonPerDay
MegaukGallonPerDay

@@ -40,5 +40,7 @@ public class VolumePerLengthTests : VolumePerLengthTestsBase
protected override double LitersPerKilometerInOneCubicMeterPerMeter => 1E6;

protected override double LitersPerMillimeterInOneCubicMeterPerMeter => 1;

protected override double UsGallonsPerMileInOneCubicMeterPerMeter => 4.251439077933434e5;
Copy link
Owner

@angularsen angularsen Mar 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Owner

@angularsen angularsen Mar 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you used the "miles per gallon" constant, instead of "gallons per mile".

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found 4.25143707430272e5 (source)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tmilnthorp
Fixed.

Common/UnitDefinitions/VolumePerLength.json Outdated Show resolved Hide resolved
Copy link
Owner

@angularsen angularsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the constants were not correct, please double check

My bad, they were correct, but I propose improving the precision

@t03apt t03apt changed the title Add UsGallonsPerMile Add UsGallonsPerMile, ImperialGallonPerMile, MegaukGallonPerDay, MegausGallonPerDay Mar 13, 2023
@t03apt t03apt requested a review from angularsen March 13, 2023 23:38
Copy link
Owner

@angularsen angularsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I pushed minor improvements to the test constants and added links to the sources.

UnitsNet.Tests/CustomCode/VolumeFlowTests.cs Outdated Show resolved Hide resolved
UnitsNet.Tests/CustomCode/VolumeFlowTests.cs Outdated Show resolved Hide resolved
@angularsen angularsen merged commit f146813 into angularsen:master Apr 1, 2023
@t03apt
Copy link
Contributor Author

t03apt commented Apr 1, 2023

Looks good, I pushed minor improvements to the test constants and added links to the sources. @angularsen

I am sorry about removing wolframalpha comments during merge. Should I add wolframalpha links to any new units in the future? Is there a guideline about it that I missed?

@angularsen
Copy link
Owner

No problem, I added them back.

There is no guideline about documenting sources, but I realized this is probably a Good Thing (tm) since we have had a bit more focus on preserving high precision in conversion functions lately.

I'll add a note about it in the wiki for adding a new unit.

@angularsen
Copy link
Owner

Sorry for the late turnaround, we usually get things done quicker. Work and family has been eating all my time the past few weeks.

Thanks for your effort! Nuget should be out shortly.

Release UnitsNet/5.10.0 · angularsen/UnitsNet

@angularsen
Copy link
Owner

Added info about documenting source of value:
https://github.com/angularsen/UnitsNet/wiki/Adding-a-New-Unit#4-fix-generated-test-stubs-to-resolve-compile-errors-tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants