-
Notifications
You must be signed in to change notification settings - Fork 394
Add units to ForceChangeRate, PressureChangeRate, fix abbreviations #914
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
… and fix translations Add new units: - ForceChangeRate: PoundForcePerMinute and PoundForcePerSecond - PressureChangeRate: PoundForcePerSquareInchPerSecond and PoundForcePerSquareInchPerMinute Add new abbreviations: - Pressure: PoundForcePerSquareInch (ksi) Fixed translation: - PressureChangeRate : Russians abbreviations for units per minutes
Codecov Report
@@ Coverage Diff @@
## master #914 +/- ##
========================================
+ Coverage 82.7% 82.8% +0.1%
========================================
Files 291 291
Lines 44035 44148 +113
========================================
+ Hits 36415 36528 +113
Misses 7620 7620
Continue to review full report at Codecov.
|
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.
Looks almost good to go, just a few observations.
…undsForce definition Remove the KiloPoundForce unit definition replaced by Kilo prefixe in PoundForce (and abbreviations for this prefixe)
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.
Just one minor question, then this should be ready.
"Abbreviations": [ "кипф", "койка", "К" ] | ||
} | ||
] | ||
}, |
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.
Nice that you merged these two definitions. Support for Prefixes was added long after this unit was added.
{ | ||
"Culture": "en-US", | ||
"Abbreviations": [ "psi/s", "lb/in²/s" ], | ||
"AbbreviationsForPrefixes": { "Kilo": ["kipf/in²/s", "ksi/s" ]} |
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.
Is ksi/s more widely used than kipf/in2/s? If so, consider swapping the order of these. Seems more consistent with psi/s being the first choice, too.
The first abbreviation is the one used by ToString() when no custom formatting is specified: https://github.com/angularsen/UnitsNet/wiki/String-Formatting
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 think you're right (that's what I think too), but actually i just copied the abbreviations from PoundForcePerSquareInch in Pressure.json
To keep it logical, i think we should change PoundForcePerSquareInch definition too, but as you said, the first abbreviation is used by ToString(), so it could introduce some breaking (even if i never see "kipf/in²" in any standard/document).
What do you think i should do?
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 think we can risk a minor breaking change and swap the order of Pressure too, for the sake of consistency.
UnitsNet.WindowsRuntimeComponent/GeneratedCode/UnitAbbreviationsCache.g.cs
Show resolved
Hide resolved
…te, KilopoundForcePerSquareInchPerSecond and KilopoundForcePerSquareInch (ksi and kipf/in²) ksi is now the first (and default ) abbreviation rather than kipf/in²
Thanks a lot! Nuget is on the way out. |
Add new units:
Add new abbreviations:
Fixed translation: