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

Cleanup of OptionStrategy #46

Closed
wants to merge 3 commits into from
Closed

Cleanup of OptionStrategy #46

wants to merge 3 commits into from

Conversation

lukasz-pyrzyk
Copy link

I spent a couple of minutes to find out that i cannot use OptionAttribute with types different than bool. That is why I've added helpful information for another people.

Also, i did some general cleanup of whole class.

@Nick-Lucas
Copy link
Owner

Nick-Lucas commented May 10, 2017

Hey Lukasz,

Thanks a lot for sending this in, your updated exception message is a really great change.

Unfortunately I won't be able to merge this in, because of the style changes, which no longer match the style used throughout this project.

However, I'm currently planning a round of changes to improve exception messages and make some bad-scenarios clearer to both developers and users, so will definitely bring over your change when I do this.
Of course if you want to slim down the PR to just the message improvement, I'll happily merge this in for you.

Nick :)

This reverts commit c1f5c64.
@lukasz-pyrzyk
Copy link
Author

Hi.

Sure, if you are going to improve exception messages i think my change is redundant :)

Thanks

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

Successfully merging this pull request may close these issues.

2 participants