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

Adds support for non constant definitions #18

Merged
merged 1 commit into from Mar 14, 2017

Conversation

@laertispappas
Copy link
Collaborator

laertispappas commented Mar 14, 2017

Addresses issue #17 and adds support for non constant definitions. Although there is no example in the docs where we define enums in lowercase some are using Ruby::Enum as follows:

class State
  include Ruby::Enum

  define :created, 'Created'
end
@laertispappas

This comment has been minimized.

Copy link
Collaborator Author

laertispappas commented Mar 14, 2017

@dblock Could you please review this?

@dblock

This comment has been minimized.

Copy link
Owner

dblock commented Mar 14, 2017

I don't love relying on an exception. Maybe we can check the value being passed in? If it's an all uppercase string we do one thing, otherwise we do another? I'm not sure it's better though, what do you think?

@laertispappas

This comment has been minimized.

Copy link
Collaborator Author

laertispappas commented Mar 14, 2017

Agree! I thought about it too but I didn't want o make things more complex and add if decisions based on the first character of the enum name. We could isolate the const_set in a begin rescue block but still it's cryptic to rely on the exception. I will proceed with you proposal. Thanks

@laertispappas laertispappas force-pushed the laertispappas:non_constant_definitions branch from df783ab to 1e9147f Mar 14, 2017
@dblock dblock merged commit fdbbd5d into dblock:master Mar 14, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dblock

This comment has been minimized.

Copy link
Owner

dblock commented Mar 14, 2017

This looks good, merged. Maybe release?

@laertispappas

This comment has been minimized.

Copy link
Collaborator Author

laertispappas commented Mar 15, 2017

Sure I will prepare it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.