-
Notifications
You must be signed in to change notification settings - Fork 400
Added "Deca", "Deci" and "Hecto" prefixes for "UsGallon" unit with tests #779
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
Conversation
…tishThermalUnitsInOneJoule
…angularsen-master # Conflicts: # Common/UnitDefinitions/Energy.json # Common/UnitDefinitions/Mass.json # Common/UnitDefinitions/Power.json # Common/UnitDefinitions/Volume.json # UnitsNet.Tests/CustomCode/EnergyTests.cs # UnitsNet.Tests/CustomCode/FlowTests.cs # UnitsNet.Tests/CustomCode/MassFlowTests.cs # UnitsNet.Tests/CustomCode/MassTests.cs # UnitsNet.Tests/CustomCode/PowerTests.cs # UnitsNet.Tests/CustomCode/VolumeTests.cs # UnitsNet.Tests/GeneratedCode/EnergyTestsBase.g.cs # UnitsNet.Tests/GeneratedCode/FlowTestsBase.g.cs # UnitsNet.Tests/GeneratedCode/MassFlowTestsBase.g.cs # UnitsNet.Tests/GeneratedCode/MassTestsBase.g.cs # UnitsNet.Tests/GeneratedCode/PowerTestsBase.g.cs # UnitsNet.Tests/GeneratedCode/VolumeTestsBase.g.cs # UnitsNet/GeneratedCode/Enums/EnergyUnit.g.cs # UnitsNet/GeneratedCode/Enums/FlowUnit.g.cs # UnitsNet/GeneratedCode/Enums/MassFlowUnit.g.cs # UnitsNet/GeneratedCode/Enums/PowerUnit.g.cs # UnitsNet/GeneratedCode/Enums/VolumeUnit.g.cs # UnitsNet/GeneratedCode/Extensions/Number/NumberToEnergyExtensions.g.cs # UnitsNet/GeneratedCode/Extensions/Number/NumberToFlowExtensions.g.cs # UnitsNet/GeneratedCode/Extensions/Number/NumberToMassExtensions.g.cs # UnitsNet/GeneratedCode/Extensions/Number/NumberToMassFlowExtensions.g.cs # UnitsNet/GeneratedCode/Extensions/Number/NumberToPowerExtensions.g.cs # UnitsNet/GeneratedCode/Extensions/Number/NumberToVolumeExtensions.g.cs # UnitsNet/GeneratedCode/UnitClasses/Energy.g.cs # UnitsNet/GeneratedCode/UnitClasses/Flow.g.cs # UnitsNet/GeneratedCode/UnitClasses/Mass.g.cs # UnitsNet/GeneratedCode/UnitClasses/MassFlow.g.cs # UnitsNet/GeneratedCode/UnitClasses/Power.g.cs # UnitsNet/GeneratedCode/UnitClasses/Volume.g.cs # UnitsNet/GeneratedCode/UnitSystem.Default.g.cs # UnitsNet/UnitDefinitions/Flow.json # UnitsNet/UnitDefinitions/MassFlow.json
Codecov Report
@@ Coverage Diff @@
## master #779 +/- ##
==========================================
+ Coverage 63.82% 63.84% +0.02%
==========================================
Files 278 278
Lines 41477 41507 +30
==========================================
+ Hits 26471 26501 +30
Misses 15006 15006
Continue to review full report at Codecov.
|
Hi, thanks for contributing. I have to ask though, are you sure these units are actually widely used? I can't seem to find much on Google and it seems a bit weird to me that metric prefixes (deci, kilo, deca) etc are used for US units. It would be great to see some URLs pointing to references or actual usage of these units before merging this as I suspect it is maybe not used much. Do you use these units yourself? In what domain/context? Best, |
Hi, @angularsen First of all, thank you so much for creation of such amazing library! I can not express well enough how much I love it. 2nd, I work as an developer on Energy and Utility Management system that targets US market. A plenty of US utility companies use 'cGal'/'100 Gal' and 'kGal'/'1000 Gal' units in theirs reports and DBs for utility types as 'Water', 'Diesel Fuel', 'Oil' and so on. More than that, Google seems to be aware of this units and looks like people search for such conversions on regular basis. However I totally agree that 'DecaGallon' it rather something exotic, so plz let me know if you would like to remove it and I'll get rid of this in no time. |
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 a lot for the references, very helpful. I'm sold then :-) Hecto-unit can stay, I don't mind since it is so close to deca and deci.
Looks good to me. I'll wait for your reply before merging this.
"FromUnitToBaseFunc": "x*0.00378541", | ||
"FromBaseToUnitFunc": "x/0.00378541", | ||
"Prefixes": [ "Kilo", "Mega" ], | ||
"Prefixes": [ "Deca", "Deci", "Hecto", "Kilo", "Mega" ], |
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 mentioned reports using 'cGal'/'100 Gal' and 'kGal'/'1000 Gal'.
Please note:
- The abbreviations used by
ToString()
will in this PR bedgal (U.S.)
andhgal (U.S.)
etc. Volume.Parse("5 cGal (U.S.)")
will throw an exception, because parsing is currently case sensitive.
If you really want ToString()
and Parse()
to support using uppercase Gal
, then you need to add that Gal (U.S.)
abbreviation to the end of the Abbreviations
array below. It would still need the (U.S.)
suffix to be consistent with our naming conventions though..
To use a specific abbreviation in ToString()
, take a look at custom string formatting.
If you are fine with the existing lowercase gal (U.S.)
abbreviation, then I recommend not adding Gal (U.S.)
unless it will actually be used.
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.
#781 is now merged, so this is point is no longer relevant :-) This PR is then good to go!
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 Sorry for delay with my reply. You see, in our system exists a few different names for the same unit. First company names it "kGal", the second - "100 Gal", the third - "Hundred Gallons" and so on. Thus I don't think that there is any standard name of those units in the States, and if there is actually one, looks like no one really respects it. So I would leave it as it is now. :-)
I've had this nagging in the back of my mind for some time, so I finally got a PR out to support case insensitive parsing that I mentioned above. If that is merged, then it should be able to parse |
Nuget is on the way out. |
Excellent, thanks for the update.
|
No description provided.