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

Change the return type of DehumanizeTo<TTargetEnum> to TTargetEnum #60

Closed
MehdiK opened this issue Jan 16, 2014 · 9 comments
Closed

Change the return type of DehumanizeTo<TTargetEnum> to TTargetEnum #60

MehdiK opened this issue Jan 16, 2014 · 9 comments
Labels

Comments

@MehdiK
Copy link
Member

MehdiK commented Jan 16, 2014

It's a pain to have to cast back to the target enum every time.

@maximn
Copy link
Contributor

maximn commented Jan 26, 2014

Now in case of no match, it returns null. After the change, it won't be possible.
What do you expect to happen instead of the null?

@MehdiK
Copy link
Member Author

MehdiK commented Jan 26, 2014

It could throw. You're asking to dehumanize a string into a wrong enum (or a wrong string into an enum) and an exception shouldn't be too much of a surprise.

That said, I was actually going to implement it as a generic method in the beginning and couldn't - need generic constraint on enum (doable using Fody tho)! This was recently raised by a user on twitter and I created here if anyone is willing to give it a shot :)

@maximn
Copy link
Contributor

maximn commented Jan 26, 2014

Thought about doing a constraint something like" : struct, IComparable, IFormattable, IConvertible"
What do you think?

Regarding the excpetion, I'm not so familiar with the project, is there already any exception in the project that can be used in this case?

@MehdiK
Copy link
Member Author

MehdiK commented Jan 26, 2014

Yeah, the constraint is cool. There is proper enum constraint here; but I'd rather Humanizer stay independent as much as possible.

No existing exception. I'd create an explicit exception for this case.

@maximn
Copy link
Contributor

maximn commented Jan 26, 2014

Done. Sent a pull request, please check it out and let me know what do you think.

Another thing, I wasn't sure how to run the tests so it'll be nice to add a document that explains how to run them.

@MehdiK
Copy link
Member Author

MehdiK commented Jan 26, 2014

Awesome. Thanks. Will do now.

WRT running tests, they're just xUnit tests that you could run using TD.Net, R# (plus the xUnit plugin) or xUnit runner. They're also run with automatic PR verification through TeamCity as explained here. You can check your PR status on your PR now. It's green :)

@MehdiK
Copy link
Member Author

MehdiK commented Jan 26, 2014

Thanks for the contribution. There were a few small things I raised on the PR. I'd appreciate if you could fix them and I will merge it in.

@maximn
Copy link
Contributor

maximn commented Jan 26, 2014

OK. Fixed it and added to the test.
I'll let you create the new exception.

@MehdiK
Copy link
Member Author

MehdiK commented Jan 27, 2014

Addressed in #69

@MehdiK MehdiK closed this as completed Jan 27, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants