Skip to content
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

ExistsIn validation fails with non-int backed Enums #139

Open
fsateler opened this issue Apr 7, 2016 · 4 comments
Open

ExistsIn validation fails with non-int backed Enums #139

fsateler opened this issue Apr 7, 2016 · 4 comments

Comments

@fsateler
Copy link
Contributor

fsateler commented Apr 7, 2016

The currently released version (2.1.0), currently casts enums to int when fetching possible values, to be able to compare the string representation.

This fails with short or long backed enums with an InvalidCastException.

This could be fixed by using Convert.ToInt32 instead. But I wonder if this is the right approach at all? I would have expected that the ExistsIn attribute refers to a property that has the same type as the target property. If I'm interpreting the code correctly, the following code would fail:

[ExistsIn("Values", "Key", "Value")]
public MyEnum Property {get;set;}
public Dictionary<MyEnum,string> Values = ...;

Because only the values fetched from Values are converted to int, but the Property value is not.

@robdmoore
Copy link
Member

Weird. Off the top of my head I'm not sure why we are converting to an int. It makes more sense to me to always deal with the string value of the enum not the int.

There must be a reason why we have the cast though.

If you can work out why it's done that way and a solution that keeps as string or fixes your problem in another way I'm happy to take a PR.

Out of curiosity though, why would you use an enum backed by something that isn't an int? I must admit, I've never come across that before in any codebases I've seen :P

@fsateler
Copy link
Contributor Author

A possible solution would be to use Convert.ToInt32. It should be a bit slower, but this part is not performance critical anyway.

Out of curiosity though, why would you use an enum backed by something that isn't an int? I must admit, I've never come across that before in any codebases I've seen :P

It is backed by a short, because that is how it is stored in the database...

@robdmoore
Copy link
Member

Fair enough.

I'm comfortable accepting a PR that does a Convert.ToInt32, but I'm still curious why it even casts them to an int in the first place (I realise I'm probably the one that wrote it, but I can't remember specifics since it was years ago now haha).

@Devika-Rudagi
Copy link

Hi, is this issue still open? I would like to work on it

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

No branches or pull requests

3 participants