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 to subclass an Ruby::Enum #13

Merged
merged 5 commits into from
Feb 20, 2017

Conversation

laertispappas
Copy link
Collaborator

This PR addresses the feature described in #3. Ruby::Enum now supports subclassing by inheriting all enums defined in the parent class. New enums may be defined in subclasses of course. I also made fixed some typos in the README and fixed the code to raise the correct exception when a Ruby::Enum class has not defined any enums (empty class ).

All subclasses of type S that inherit from type T
should have all enums defined in parent class T.
@dblock
Copy link
Owner

dblock commented Feb 20, 2017

Copy link
Owner

@dblock dblock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, just need to fix the build and minor text, please.

CHANGELOG.md Outdated
@@ -2,6 +2,10 @@

* Your contribution here.

### 0.6.1 (Next)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's one just like that above, remove.

CHANGELOG.md Outdated
@@ -2,6 +2,10 @@

* Your contribution here.

### 0.6.1 (Next)

* [#3](https://github.com/dblock/ruby-enum/pull/13): Adds support for sub classing an Enum- [@laertispappas](https://github.com/laertispappas).
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a space before -.

@@ -43,8 +45,10 @@ def validate_value!(value)
end

def const_missing(key)
if @_enum_hash[key]
if @_enum_hash && @_enum_hash[key]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe .key?(key)? I believe it's faster. Otherwise assign first to avoid the double lookup:

r = @_enum_hash[key] if @_enum_hash
if r
  r.value
elsif ...

Given a class S where the method define is not called, @_enum_hash
will be nil. In order to not raise a undefined method [] for nil
class in const_missing I added the behavior to check for the presence
if the `@_enum_hash` first before trying to access the enums hash.
@laertispappas
Copy link
Collaborator Author

Thanks for the feedback. I have reviewed the changes and rebased interactive. Why did the build brake now with this commits? Shouldn't it have broken on previous builds as well?

@dblock
Copy link
Owner

dblock commented Feb 20, 2017

There was a new release of Rake, and we didn't lock anything here.

@@ -1,6 +1,6 @@
### 0.6.1 (Next)

* Your contribution here.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put this back please.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:). Sure.

@dblock
Copy link
Owner

dblock commented Feb 20, 2017

One more small thing in changelog ...

Do you want to help out with this gem? Would love to give you r/w access and rubygems to make a release (what's your rubygems email?).

Rake 11.0.1 removes the last_comment method which Rails 2.3 rspec-core (< 3.4.4) uses.
Therefore until/if a patch is released we need to pin rake to an older version in Gemfile:
@laertispappas
Copy link
Collaborator Author

@dblock Sure I will to my best. [email protected]

@dblock dblock merged commit c16eadd into dblock:master Feb 20, 2017
@dblock dblock mentioned this pull request Feb 20, 2017
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