-
Notifications
You must be signed in to change notification settings - Fork 384
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
Feature: Added MolarMass Quantity Type #305
Conversation
NanogramPerMole to KilogramPerMole convertion tests were failing, so we set the tolerance to 1e-3 like in massTests
}, | ||
{ | ||
"SingularName": "PoundPerMole", | ||
"PluralName": "PoundsPerMoles", |
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'm fixing this to PoundsPerMole
and GramsPerMole
.
{ | ||
"Culture": "en-US", | ||
"Abbreviations": [ "lb/mol" ], | ||
"AbbreviationsWithPrefixes": [ "KLbs/mol", "MLbs/mol" ] |
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.
Kilo should be lowercase klbs
. Also, I think I prefer the singular form when it comes to abbreviations. I know there are varying practices here, but unless klbs/mol
is clearly preferred over klb/mol
in this domain then I think we should use singular form to stay consistent in the library.
I took the liberty of editing my suggestions in directly. Please review if you agree, then this should be ready to merge. |
I can't push the generated code apparently, so if you could run |
I reviewed your changes and didn't find any problem. I think it's ready to merge. |
{ | ||
"Culture": "ru-RU", | ||
"Abbreviations": [ "г/моль" ], | ||
"AbbreviationsWithPrefixes": [ "нг/моль", "мкг/моль", "мг/моль", "сг/моль", "дг/моль", "даг/моль", "гг/моль", "кг/моль" ] |
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.
Can we drop these prefixes and rely on the auto-generated prefixes, like the unit below?
It seems to me they only add the standard prefix letter here.
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.
Never mind, we can do that separately. No need to delay the PR over 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.
Yes, I thought those prefixes were necessary. But if it's not, let's drop them.
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.
Actually, it turns out only English is supported by the scripts. It should be trivial to extend the script to support other languages too as long as they simply prepend a certain string for each prefix. Russian seems to do this, but it is not a language I am familiar with.
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 am going to open an issue about this to remember later.
Awesome, looks good to me! |
Squashed commit of the following: commit 6e23232 Author: Andreas Gullberg Larsen <anjdreas@gmail.com> Date: Sat Nov 11 12:33:12 2017 +0100 Fix headers Wrong year and use (c) instead of copyright symbol since git and powershell frequently messes up the encoding when using it. commit c102e3e Author: Andreas Gullberg Larsen <anjdreas@gmail.com> Date: Sat Nov 11 12:25:18 2017 +0100 Regenerate code from PRs commit c1f83dd Author: Ferit Tunçer <ferit@lavabit.com> Date: Sat Nov 11 13:55:29 2017 +0300 Add quantity Entropy (#312) commit 90d5f93 Author: Ferit Tunçer <ferit@lavabit.com> Date: Sat Nov 11 13:43:12 2017 +0300 Fix LapseRate Units (#321) commit c94c1d2 Author: Ferit Tunçer <ferit@lavabit.com> Date: Fri Nov 10 20:43:44 2017 +0300 Delete duplicate quantity SubstanceAmount (#317) commit 471d2fc Author: Andreas Gullberg Larsen <anjdreas@gmail.com> Date: Thu Nov 9 21:25:20 2017 +0100 Add Equals(T other, T maxError), obsolete Equals(T other) for Double quantities Equality comparison is not safe with System.Double as internal representation. Decimal quantities (Power, Information) still allow equality. Add test on new Equals() method. commit e0eb3f0 Author: Andreas Gullberg Larsen <anjdreas@gmail.com> Date: Thu Nov 9 19:57:32 2017 +0100 UnitsNet: 3.78.0 commit 041a53e Author: Ferit Tunçer <ferit@lavabit.com> Date: Thu Nov 9 21:54:51 2017 +0300 Add quantity LapseRate (#316) commit 8a4d648 Author: Andreas Gullberg Larsen <anjdreas@gmail.com> Date: Tue Nov 7 15:34:47 2017 +0100 Fix email address commit aa9a99a Author: Ferit Tunçer <ferit@lavabit.com> Date: Tue Nov 7 23:43:11 2017 +0300 Add micropascalseconds (#314) commit 0a585b8 Author: Ferit Tunçer <ferit@lavabit.com> Date: Tue Nov 7 17:45:26 2017 +0300 Add MolarEntropy (#310) commit e51cd52 Author: Ferit Tunçer <ferit@lavabit.com> Date: Tue Nov 7 17:04:30 2017 +0300 Add MolarEnergy (#309) commit 1fad2bb Author: Ferit Tunçer <ferit@lavabit.com> Date: Tue Nov 7 17:02:05 2017 +0300 Add AmountOfSubstance (#304) commit 6800561 Author: Ferit Tunçer <ferit@lavabit.com> Date: Tue Nov 7 08:53:13 2017 +0300 Add MolarMass Quantity Type (#305) * added MolarMass * added russian abbreviations NanogramPerMole to KilogramPerMole convertion tests were failing, so we set the tolerance to 1e-3 like in massTests.
Implemented molar mass which we discussed in #303
Fixes #303