angularsen / UnitsNet

Makes life working with units of measurement just a little bit better.
https://www.nuget.org/packages/UnitsNet/
MIT No Attribution
2.63k stars 383 forks source link

Custom abbreviation doesn't ToString() well #577

Closed gregsdennis closed 5 years ago

gregsdennis commented 5 years ago

Problem

If I add a custom abbreviation:

UnitAbbreviationsCache.Default.MapUnitToAbbreviation(VolumeUnit.UsCustomaryCup, "Cup");

it parses just fine. However, when I call .ToString(), I still get the default abbreviation of "".

NOTE I understand that there is ambiguity between "cup" units, and that's why it's defaulted to an empty string. I'm using the code above to override this default and use US Customary Cup for my app.

Cause

The above method merely appends the abbreviation to the list of acceptable values.

https://github.com/angularsen/UnitsNet/blob/87dc3c79e6b7d9b5719c6b310c5c33bd98349a6c/UnitsNet/CustomCode/UnitValueAbbreviationLookup.cs#L72-L82

However, the .ToString() methods on classes only use the first abbreviation in the list.

https://github.com/angularsen/UnitsNet/blob/87dc3c79e6b7d9b5719c6b310c5c33bd98349a6c/UnitsNet/CustomCode/UnitAbbreviationsCache.cs#L194

Proposal

Add a new method

void MapUnitToDefaultAbbreviation<TUnitType>(TUnitType unit, string abbreviation) where TUnitType : Enum
{
     ...
}

that would insert the new abbreviation at the beginning of the list rather than appending it to the end. Then when .ToString() is called, it can still take the first abbreviation.

tmilnthorp commented 5 years ago

I wonder if it would be better to do some of the ToString work that we talked about to support this?

gregsdennis commented 5 years ago

@tmilnthorp are you referencing some other issue? I looked for one that would fit my change, but I couldn't find one close enough.

tmilnthorp commented 5 years ago

I think it was in a PR, sorry! There was some discussion on #545. I forgot to create an issue on it.

You can see how you could provide any abbreviation # in the list with this idea.

tmilnthorp commented 5 years ago

I've added #579

angularsen commented 5 years ago

@tmilnthorp But doesn't ToString() custom formats solve a different problem? Mapping custom unit abbreviations is about adding abbreviations not already in the library, or changing the default abbreviations. A custom format in ToString() doesn't provide this as far as I can tell. Also abbreviation number is a bit risky, as I can't vouch for the ordering not being changed at some point.

Mapping unit abbreviations also allows you to define your own unit enums (we are dealing with Type and int after all) in order to later parse them, but that is maybe a very niche usecase since we don't support custom quantities so I guess I don't see a big value there.

gregsdennis commented 5 years ago

I agree with @angularsen. This seems mostly related, but still quite disjoint.

tmilnthorp commented 5 years ago

For changing the default, I think you would indeed have to change the order regardless. I seem to recall the ordering being delicate for something when we refactored the parsing code.

But if you wanted to extend, but not change the default, how would you specify which unit ToString outputs without a numeric specifier somehow?

angularsen commented 5 years ago

I guess you can't :-/ At least I don't see a good way for that to work.

The main idea for having multiple abbreviations to begin with is to support parsing all of them, I didn't originally intend for it to be used for ToString() other than the default one. I mean, in a custom format pattern, you can always type out the abbreviation yourself if you want something else. I just have a hunch that using a numeric value here will break at some point when we figure we need to change the order or remove one of the abbreviations for any reason.