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

values: prepend superclass values #29

Merged
merged 11 commits into from
Jan 29, 2021
Merged

values: prepend superclass values #29

merged 11 commits into from
Jan 29, 2021

Conversation

gi
Copy link
Collaborator

@gi gi commented Jan 27, 2021

When enumerating values for a subclass, the superclass values are prepended.

Fixes #27.

class A
  include Ruby::Enum
  define :X, 'X'
end

class B < A
  define :Y, 'Y'
end

B.values() == [B::X, B::Y]

When enumerating values for a subclass, the superclass values are prepended.

Fixes #27.
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.

Thanks!

  • update CHANGELOG
  • bump version to 0.9.0
  • document values behavior in README

lib/ruby-enum/enum.rb Outdated Show resolved Hide resolved
spec/ruby-enum/enum_spec.rb Outdated Show resolved Hide resolved
spec/ruby-enum/enum_spec.rb Show resolved Hide resolved
gi added 4 commits January 27, 2021 10:25
The spec file violates the Metrics/BlockLength cop and always needs to be updated.
This excludes the file from being evaluated against that rule so that other blocks
may still adhere to a normal block length configuration.
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.

More comments below.

CHANGELOG.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
lib/ruby-enum/enum.rb Outdated Show resolved Hide resolved
@gi
Copy link
Collaborator Author

gi commented Jan 27, 2021

I think because we're changing behavior here in significant ways, I think we need an UPGRADING file

Could this just go into the changelog? I worry that the UPGRADING file is going to look exactly like the CHANGELOG. Perhaps, for this simple case, a "Breaking Changes" section in the changelog version's entry?

What did you have in mind for the content?

@dblock
Copy link
Owner

dblock commented Jan 27, 2021

I think because we're changing behavior here in significant ways, I think we need an UPGRADING file

Could this just go into the changelog? I worry that the UPGRADING file is going to look exactly like the CHANGELOG. Perhaps, for this simple case, a "Breaking Changes" section in the changelog version's entry?

This is more of a convention thing from dozens of projects - sometimes these get very long and full of before/after examples, so we haven't been putting long explanations about upgrading into changelog.

What did you have in mind for the content?

I think something like enum classes now inherit values, for example ... with before: and after: output. Keep the format similar to the UPGRADING.md I linked

(thank you)

define :BLUE, 'BLUE'
end

class RainbowColors < PrimaryColors
Copy link
Owner

Choose a reason for hiding this comment

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

The example is super clear, this works.

I do have a question. In the scenario with colors inheritance is the wrong object oriented pattern to use. Rainbow colors don't "inherit" primary colors, they are a combination of primary colors and other colors. In this case we would want a module and mixins. Should we allow class PrimaryColors be module PrimaryColors, extend Ruby::Enum, then include PrimaryColors into RainbowColors or something like that? ... for another PR.

Maybe there's a better OOO example for README that we can use in a separate update? WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah. Semantically, it makes more sense to include the PrimaryColors in the RainbowColors class instead of inheriting them.

Though, I see this is a minor implementation detail. Defining an enum doesn't actually create an instance of that class: Colors::RED isn't an instance of Ruby::Enum or Colors. It's a String or Symbol.

There's little difference between inheriting from a class and including a module other than the Ruby semantics. Both options include the base/included class in the class hierarchy.

Regardless, I think this could be an update for another PR.

Copy link
Owner

Choose a reason for hiding this comment

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

100%

@dblock dblock merged commit 8792ca1 into dblock:master Jan 29, 2021
@dblock
Copy link
Owner

dblock commented Jan 29, 2021

Merged, thank you! Do you want to help out with maintenance of this gem @gi? Maybe make the next release. If yes, email dblock[at]dblock[dot]org with your rubygems username/email and I'll add you.

@gi gi deleted the issue-27/subclass-values branch February 1, 2021 05:12
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.

values class method does not read inherited enums
2 participants