Support multiple abbreviations #658
Conversation
Added the following Concentration Units: - MassConcentration: SI = kg/m3, typically mg/l - VolumeConcentration : dimensionless, typically % - MassFraction: SI = kg/kg, typically mg/kg Modified the existing Molarity unit: - Some operations that were originally based on the Density units now use the MassConcentration units instead (Note: despite the fact that they share the same measurement units- the Density is a distnct QuantyType from the MassConcentration) - Removed all operators involving Molarity from the Density units Defined some basic operations that were missing from the AmountOfSubstance/MolarMass/Mass units Defined the triangular operations involving Mass/Molar/Volume concentrations (& the corresponding component's Density & MolarMass) All unit tests included most were defined by actual chemists(which I AM NOT). Note: one of the tests (QuantityIFormattableTests.VFormatEqualsValueToString)- was failing on my machine- it passes if I add CultureInfo.CurrentUICulture to the value.ToString() - as I presume was the intended behavior
- updated liter abbreviations for g/l, g/dl, g/ml & kg/l to uppercase 'L' (TODO Density?) - added base units to all units in MassConcentration & Molarity (TODO Density?)
- corrected the BaseUnits for MassConcentration - marked the invalid methods from Molarity/Density as Obsolete (were previously omitted) - some cosmetic changes to the Unit Tests
- MolesPerLiter: fixed the BaseUnits (default) to Deimeter/Mole - Molar: removed in favor of using the alternative abbreviation 'M" - MolarityTests - OneMilliMolarFromStringParsedCorrectly skipped while awaiting fix for #344
- added a KnownQuantities class with a few constants that were used in multiple tests - replaced the usages in MassConcentrationTests MolarityTests * VolumeConcentrationTests
- converted two of the MassConcentration tests to using Theory + InlineData
- removed BaseUnits from GramPerDeciliter(not exact + overlap), kept it in GramPerLiter (as exact & non-overlapping), also kept it for GramPerMilliliter(exact + overlapping) because I thought it would be useful to have at least one such case for future testing - moved the Mass/MolarMass operator to the Mass class (removing the MolarMass.extra) - all tests refactored using Theory + Inline Data - moved one or two tests to the appropriate .Test file - removed a few redundant tests
- Types.psm1: type of AbbreviationsWithPrefixes changed to object[]
- GenerateUnits.ps1: Add-PrefixUnits now iterates over all abbreviations (incl. with prefix)
- Duration: added the cyrilic equivalent to "s": "sec" ("с", "сек") to both the main and the prefixed abbrviations
- DurationTests: added a Theory with some of the main test cases for parsing seconds & milliseconds
- MolarityTests: removed the Skip for the awaiting test
| @@ -212,17 +212,17 @@ public sealed partial class UnitAbbreviationsCache | |||
| ("en-US", typeof(DurationUnit), (int)DurationUnit.Hour, new string[]{"h", "hr", "hrs", "hour", "hours"}), | |||
| ("ru-RU", typeof(DurationUnit), (int)DurationUnit.Hour, new string[]{"ч"}), | |||
| ("en-US", typeof(DurationUnit), (int)DurationUnit.Microsecond, new string[]{"µs"}), | |||
| ("ru-RU", typeof(DurationUnit), (int)DurationUnit.Microsecond, new string[]{"мкс"}), | |||
| ("ru-RU", typeof(DurationUnit), (int)DurationUnit.Microsecond, new string[]{"мкс мксек"}), | |||
There was a problem hiding this comment.
This doesn't seem right, it's a single string vs multiple strings.
I realize these are the Windows Runtime Component files. WRC is kind of deprecated and only maintained on the back-burner, so it has its own copy of the codegen files that are by now outdated and slightly different. However, it would currently break the existing abbreviations so we need to fix that.
Could you copy the script change to codegen scripts in UnitsNet.WindowsRuntimeComponent\Scripts folder and regenerate? I would prefer that C# files don't change as a result of this PR, but if it's easiest that WRC too receives the new abbreviations then that is fine too.
There was a problem hiding this comment.
Hmm, this is strange. Why did the AppVeyor checks pass the first time? Shouldn't have my tests failed?
Code generated looks fine now, but I don't know- how do you really make sure..
There was a problem hiding this comment.
AppVeyor regenerates the code too, since often when merging multiple PRs the committed generated code is out-of-date.
There was a problem hiding this comment.
Yes, but I guess it doesn't run the same tests? That's a shame, cause it would have also caught this :)
There was a problem hiding this comment.
It should run the exact same tests as when running build.bat locally - which to my knowledge should run all the tests in the solution. Isn't that the case?
I didn't quite get what you meant by the linked "this" code snippet, what should it have caught there?
There was a problem hiding this comment.
Erg, it won't let me attach the build log, but here is the highlight:
Restore completed in 26,38 ms for C:\Users\galin\Documents\GitHub\ConcentrationUnits\UnitsNet\UnitsNet.Serialization.JsonNet.Tests\UnitsNet.Serialization.JsonNet.Tests.csproj.
Retrying 'FindPackagesByIdAsyncCore' for source 'https://camo.githubusercontent.com/da308986135cea0abf9a027dc6d147d397d5e383/68747470733a2f2f696d672e736869656c64732e696f2f6e756765742f767072652f6f72636865737472612e636f72652e737667/FindPackagesById()?id='UnitsNet.Serialization.JsonNet'&semVerLevel=2.0.0'.
Response status code does not indicate success: 404 (Not Found).
Retrying 'FindPackagesByIdAsyncCore' for source 'https://camo.githubusercontent.com/da308986135cea0abf9a027dc6d147d397d5e383/68747470733a2f2f696d672e736869656c64732e696f2f6e756765742f767072652f6f72636865737472612e636f72652e737667/FindPackagesById()?id='UnitsNet.Serialization.JsonNet'&semVerLevel=2.0.0'.
Response status code does not indicate success: 404 (Not Found).
C:\Program Files\dotnet\sdk\2.1.505\NuGet.targets(114,5): error : Failed to retrieve information about 'UnitsNet.Serialization.JsonNet' from remote source 'https://camo.githubusercontent.com/da308986135cea0abf9a027dc6d147d397d5e383/68747470733a2f2f696d672e736869656c64732e696f2f6e756765742f767072652f6f72636865737472612e636f72652e737667/FindPackagesById()?id='UnitsNet.Serialization.JsonNet'&semVerLevel=2.0.0'. [C:\Users\galin\Documents\GitHub\ConcentrationUnits\UnitsNet\UnitsNet.sln]
C:\Program Files\dotnet\sdk\2.1.505\NuGet.targets(114,5): error : Response status code does not indicate success: 404 (Not Found). [C:\Users\galin\Documents\GitHub\ConcentrationUnits\UnitsNet\UnitsNet.sln]
Build FAILED.
There was a problem hiding this comment.
Weird, I have never seen this before. I initially suspected something related to SourceLink, but googling that hostname gives me this:
https://help.github.com/en/articles/about-anonymized-image-urls
Googling some more on 404 and FindPackagesByIdAsyncCore, it seems that maybe you have multiple nuget sources configured on your machine? Maybe you have added a myget source or similar?
NuGet/Home#6140
https://github.com/dotnet/cli/issues/6680
Seems like a bug with dotnet restore that is not yet fixed.
There was a problem hiding this comment.
Nice, disabling the alpha-channel source fixed the issue(I still got to keep the other sources so I guess it's kinda random). Anyway, I feel much better now. Thanks!
There was a problem hiding this comment.
I think the order matters, one of the links said it would only try the first source and ignore the others. Try moving the official nuget source as the first one.
There was a problem hiding this comment.
Yes, saw that- but for me it was already the last source. Anyway, I don't need it anymore.
| @@ -127,28 +130,40 @@ public partial class UnitAbbreviationsCache | |||
| ("en-US", typeof(BitRateUnit), (int)BitRateUnit.BitPerSecond, new string[]{"bit/s", "bps"}), | |||
| ("en-US", typeof(BitRateUnit), (int)BitRateUnit.BytePerSecond, new string[]{"B/s"}), | |||
| ("en-US", typeof(BitRateUnit), (int)BitRateUnit.ExabitPerSecond, new string[]{"Ebit/s"}), | |||
| ("en-US", typeof(BitRateUnit), (int)BitRateUnit.ExabitPerSecond, new string[]{"Ebps"}), | |||
There was a problem hiding this comment.
Well this is awesome to see, how old unit definitions now get populated new abbreviations !
And as you say, extra row vs array makes no practical difference here.
There was a problem hiding this comment.
Yeah, let's just hope people don't get surprised in a bad way as this opens the possibility for many new AmbiguousUnitParseExceptions..
Maybe there should be an explicit up-front list of those (shouldn't be difficult to auto-generate)..
There was a problem hiding this comment.
Good point, yeah that would actually be a nice list to have.
- same fix applied to the WRC project
|
Great stuff @lipchev , I'm impressed with the quality and size of your contributions lately. Would you be interested in joining as a maintainer? We are a handful, but only me and @tmilnthorp are active these days so we could definitely use the help. I don't ask much other than to help out answering issues and review PRs whenever you have the time and feel like it, no obligations. You seem like the guy to have some opinions on how this library can improve and what direction to go too, which is always nice. Let me know what you think. I would have to run it by @tmilnthorp first too. Best, |
|
I'm happy, and flattered by your confidence in me and would agree on condition that I get the full noob treatment- I am in fact still quite new to the github workflow and would love a couple of tips (like regarding the VS integration- is there something for reviewing code from the IDE- maybe a 2019 feature?) |
|
I'm quite certain you are more than qualified to help out. As for reviewing, I typically use github's web based review tools. If a PR is particularly big I might check out the branch locally and browse the code there or maybe debug some unit tests to better see what is going on. There are possibly some VS integrations to check out, but I haven't used any so I don't have any recommendations. |
|
As for git in general, I personally prefer SourceTree for reviewing local changes and staging commits. Beyond that I typically use Git for Windows' bash command line tools for most things in Git and have created a fair share of aliases to reduce the typing needed. ConEmu is a nice tool to organize multiple console windows in tabs, if you are interested in things like that. |
Merge conflict had to be ported from PowerShell to C#. UnitAbbreviationsCache now gets single entries with string arrays instead of multiple entries with single item arrays. This is an improvement, but should be functionally equivalent.
* Initial project * Add CodeGen to solution * Add DragonFruit 0.2.0-alpha.19174.3 * Remove unused after.UnitsNet.sln.targets * All codegen scripts recreated * Codegen: Minor whitespace fixes * README: Minor fix * Remove PowerShell scripts * Update build script * WRC: Remove codegen for tests It was stepping on the toes of the new codegen and WRC should not generate test code anyhow. * Remove submodules from csproj * Minor cleanup * Add initial WRC codegen * Don't call WRC PS1 scripts * WRC StaticQuantityGenerator exact match * WRC QuantityGenerator OK * WRC QuantityTypeGenerator OK * WRC UnitAbbreviationsCacheGenerator OK * Restructure folders * WRC: Remove all .ps1 scripts * Update build message for codegen * WIP * Improve xmldoc of Main method * Add option --skip-wrc * Remove dead codegen code in WRC * Re-implement Support multiple abbreviations (#658) Merge conflict had to be ported from PowerShell to C#. UnitAbbreviationsCache now gets single entries with string arrays instead of multiple entries with single item arrays. This is an improvement, but should be functionally equivalent.
|
@tmilnthorp In case you missed it, I'm hoping you can chime in on onboarding @lipchev as a collaborator. Please see #658 (comment). |
Fixes #344
Updated scripts to iterate over abbreviations
One little flaw I noticed just as I was committing was that the UnitAbbreviationsCache.g.cs generates separate rows instead of one string[]{..} but I guess that shouldn't be a problem since the way units are loaded here, just adds them in the next loop.
I won't need this fix anytime soon- so if you want you can just ignore this(I had the code stashed from before).
At the very least it brings an exhaustive test case with the addition to the duration abbreviation (I won't need those abbreviation at all, but you can keep them if you want- they are valid).