Skip to content

Units property should not include Undefined value #437

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
angularsen opened this issue May 5, 2018 · 3 comments
Closed

Units property should not include Undefined value #437

angularsen opened this issue May 5, 2018 · 3 comments

Comments

@angularsen
Copy link
Owner

angularsen commented May 5, 2018

https://github.com/angularsen/UnitsNet/blob/master/UnitsNet/GeneratedCode/Quantities/Length.g.cs#L164

Similar for all quantities, Length only chosen as an example.

It doesn't make sense to include the LengthUnit.Undefined = 0 value, since it is not meant to be used. UnitsNet will throw exception if you try to use this value for anything in methods accepting a unit value, as the intent is to help you catch bugs where you forgot to initialize the value such as when deserializing data.

@bplubell
Copy link
Contributor

If we remove Undefined, should we set the first value to be a value other than 0 (like Centimeter = 1 instead of Centimeter = 0.)?

Is this considered a breaking change, since someone may be be doing their own checking for something like LengthUnit.Undefined?

@angularsen
Copy link
Owner Author

angularsen commented Sep 20, 2018

Updated link:

/// <summary>
/// All units of measurement for the Length quantity.
/// </summary>
public static LengthUnit[] Units { get; } = Enum.GetValues(typeof(LengthUnit)).Cast<LengthUnit>().ToArray();

As you can see, I didn't mean to remove the Undefined enum value definition, I only meant to exclude it from the list of units in Length.Units property.

The motivation for this change, which I forgot to mention in the description, is that some people complained Undefined was listed when they were enumerating all Length units and I agree it's not intuitive and I don't see any purpose in keeping that value.

I would not consider this a breaking change, as I rather consider it a bug that the value was included to begin with.

The code change would be something like

public static LengthUnit[] Units { get; } = Enum.GetValues(typeof(LengthUnit)).Cast<LengthUnit>().Except(new[]{LengthUnit.Undefined}).ToArray(); 

@bplubell
Copy link
Contributor

Ah, that does make more sense. I am submitting a PR to do this - I just took your suggested solution.

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

2 participants