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

Equals implementation #20

Closed
DeveTho opened this issue Jul 5, 2018 · 12 comments
Closed

Equals implementation #20

DeveTho opened this issue Jul 5, 2018 · 12 comments

Comments

@DeveTho
Copy link
Contributor

DeveTho commented Jul 5, 2018

I was wondering if it would make sense to override and implement Equals() for the SmartEnum. Because now, if I'm checking for equality, I'm doing something like TestEnum.One.Value == myEnumVar.Value, which is not only a little more tedious, but you can also make mistakes.

If the answer is positive, I could try to do this myself and send a PR your way, if you like. :) In that case, I would have one question about what would be "the right way": two SmartEnum instances would be considered equal if their Name property is equal, right (and not necessarily their Value property, as in my example)?

Thanks by the way for providing this! I really like to use it. :)

@trevor-hackett
Copy link
Contributor

Something like TestEnum.One == myEnumVar?

Sounds like a great idea to me.

@ardalis
Copy link
Owner

ardalis commented Jul 5, 2018

I’d definitely consider a PR with this, thanks!

@DeveTho
Copy link
Contributor Author

DeveTho commented Jul 6, 2018

Cool! I'll have a look at it somewhere in the next days.

@yarrgh Exactly. :)

@trevor-hackett
Copy link
Contributor

After thinking about this a bit more doesn't TestEnum.One == myEnumVar already work?

Unless you're newing up objects outside of the static readonly properties, this should work. By default the framework compares the reference of objects unless explicitly overridden. Since they are pointed to the same instance of the object the equals check would run correctly.

@ardalis
Copy link
Owner

ardalis commented Jul 6, 2018

It probably should. Not sure if we have tests that demonstrate this is the case already. If not, we should at least add such tests, so there's no question.

@DeveTho
Copy link
Contributor Author

DeveTho commented Jul 6, 2018

Hmm, yeah, it should be the same reference. But would it be okay then to just compare by reference, and not the actual value (in case the tests would show it does work)? In that case, it should probably also be documented in the README, as it's a quite implicit thing.

Though I'm probably already thinking ahead a little. I'll try to have a look at it and let you know about and show what I found. :)

@trevor-hackett
Copy link
Contributor

To be safe, it might be best to compare the value.

I'm just remembering that an empty constructor was added to play nice with Entity Framework which, I believe, creates a new instance of the SmartEnum (see #15). In this case comparing by reference would be incorrect.

@DeveTho
Copy link
Contributor Author

DeveTho commented Jul 12, 2018

Just thought to quickly let you know I haven't further looked into it yet, sorry. But I didn't forget about it!

Also, thanks for the remarks on the EF constructor.

@DeveTho
Copy link
Contributor Author

DeveTho commented Jul 23, 2018

I just created a pull request #22 that provides this feature, I hope in a good way.

For tests, I only added some that check different instances of the SmartEnum (like when using EF). Checks by reference are already done in other tests, like SmartEnumExplicitConversion.

Feel free to let me know if there are other ideas about doing this. :)

@DeveTho
Copy link
Contributor Author

DeveTho commented Aug 22, 2018

I thought to quickly give a heads-up that if there's something I can do still regarding this issue / feature, feel free to let me know. Otherwise I'll just wait for feedback. :)

@ardalis
Copy link
Owner

ardalis commented Aug 22, 2018

What, a month seems like too long for me to look at this? ;)

ardalis pushed a commit that referenced this issue Aug 22, 2018
* Added tests for constructor (#20)

* Implemented Equals (#20)
@ardalis
Copy link
Owner

ardalis commented Aug 22, 2018

Merged - I'll try to publish a new build to Nuget soon. Thanks!

@ardalis ardalis closed this as completed Aug 22, 2018
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