Case insensitive unit parsing and conversion. - #584
Conversation
angularsen
left a comment
There was a problem hiding this comment.
Great! A few comments for you to consider.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| private static readonly string UnitTypeNamespace = typeof(LengthUnit).Namespace; | ||
| private static readonly Assembly UnitsNetAssembly = typeof(Length).GetAssembly(); | ||
| private static readonly List<Type> QuantityTypes = UnitsNetAssembly.GetTypes() |
There was a problem hiding this comment.
Since we are targeting .NET 4.0 we probably can't use IReadOnlyList<T> as I originally advised, I believe that came in .NET 4.5. I would recommend using an array and .ToArray() then, just to make it more clear that this is not supposed to be manipulated.
| if(!Enum.IsDefined(unitType, unitName)) | ||
| var eNames = Enum.GetNames(unitType); | ||
| unitName = eNames.FirstOrDefault(x => x.Equals(unitName, StringComparison.OrdinalIgnoreCase)); |
There was a problem hiding this comment.
We should probably use Enum.TryParse which includes a bool parameter to specify ignoring case. See here.
There was a problem hiding this comment.
This would also prefer this method, but I can't use that here without using reflection, which I was trying to avoid. The try parse method you linked takes a generic type argument, which means that the type has to be known at compile type. We have a Type parameter known only at run time. Is there another TryParse that you know of that takes the enum type "Type" as parameter?
There was a problem hiding this comment.
Enum.TryParse<T>(string, bool, out T) exists, but I suspect since we target .NET 4.0 is why we can't use it.
| } | ||
| [Fact] | ||
| public void ConvertByName__UnitTypeCaseInsensitive() |
There was a problem hiding this comment.
Is the double underscore intentional here and above?
| internal List<int> GetUnitsForAbbreviation(string abbreviation) | ||
| { | ||
| abbreviation = abbreviation.ToLower(); |
There was a problem hiding this comment.
I would make this a separate variable lowercaseAbbreviation to make it more clear. You can choose the name, but make the variable lower in the Add method below the same name for consistency.
| internal void Add(int unit, string abbreviation, bool setAsDefault = false) | ||
| { | ||
| // abbreviation = abbreviation.ToLower(); |
| private static bool TryGetUnitType(string quantityName, out Type unitType) | ||
| { | ||
| string unitTypeName = $"{UnitTypeNamespace}.{quantityName}Unit"; | ||
| quantityName += "Unit"; |
There was a problem hiding this comment.
Make it a variable, f.ex. unitTypeName. Also I think we can keep the example comment, but modify to // Example: "LengthUnit". I think this is helpful when reading the code.
They included more types than they should.
angularsen
left a comment
There was a problem hiding this comment.
Found some things, but I added fixes to the PR for them. Looks good to me now.
| private static bool TryGetUnitType(string quantityName, out Type unitType) | ||
| { | ||
| quantityName += "Unit"; | ||
| var quantityTypeName = quantityName += "Unit"; // ex. LengthUnit |
There was a problem hiding this comment.
It should be named unitTypeName, quantity would be Length.
Edit: I fixed it in the PR.
| if(!Enum.IsDefined(unitType, unitName)) | ||
| var eNames = Enum.GetNames(unitType); | ||
| unitName = eNames.FirstOrDefault(x => x.Equals(unitName, StringComparison.OrdinalIgnoreCase)); |
There was a problem hiding this comment.
Enum.TryParse<T>(string, bool, out T) exists, but I suspect since we target .NET 4.0 is why we can't use it.
| private static readonly string UnitTypeNamespace = typeof(LengthUnit).Namespace; | ||
| private static readonly Assembly UnitsNetAssembly = typeof(Length).GetAssembly(); | ||
| private static readonly Type[] QuantityTypes = UnitsNetAssembly.GetTypes() |
There was a problem hiding this comment.
These include a lot more types than just the quantity structs and the unit enums.
I'm adding a fix.
angularsen
commented
Jan 27, 2019
Build is failing on WRC target with my latest changes, fixing. |
angularsen
commented
Jan 27, 2019
Finally, good to go. Merging! |
angularsen
commented
Jan 28, 2019
Woops, found a bug. We lowercase the prefixes so we can no longer distinguish
As a result, |
angularsen
commented
Feb 1, 2019
Nuget 4.5.1 on the way out. https://github.com/angularsen/UnitsNet/releases/tag/UnitsNet%2F4.5.1 |
The purpose of this is to allow this library to allow for "year" and "Year" in the unit parser and UnitConverter without adding different casing to every abbreviation in the unit definitions.