Skip to content

Custom abbreviation doesn't ToString() well #577

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

Closed
gregsdennis opened this issue Dec 17, 2018 · 8 comments
Closed

Custom abbreviation doesn't ToString() well #577

gregsdennis opened this issue Dec 17, 2018 · 8 comments

Comments

@gregsdennis
Copy link
Contributor

gregsdennis commented Dec 17, 2018

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.

internal void Add(int unit, string abbreviation)
{
if(!unitToAbbreviationMap.TryGetValue(unit, out var abbreviationsForUnit))
abbreviationsForUnit = unitToAbbreviationMap[unit] = new List<string>();
if(!abbreviationToUnitMap.TryGetValue(abbreviation, out var unitsForAbbreviation))
abbreviationToUnitMap[abbreviation] = unitsForAbbreviation = new List<int>();
abbreviationsForUnit.Add(abbreviation);
unitsForAbbreviation.Add(unit);
}

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

return abbreviations.First();

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
Copy link
Collaborator

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

@gregsdennis
Copy link
Contributor Author

@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
Copy link
Collaborator

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
Copy link
Collaborator

I've added #579

@angularsen
Copy link
Owner

@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
Copy link
Contributor Author

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

@tmilnthorp
Copy link
Collaborator

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
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants