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

Add for_key() and for_value() #8

Merged
merged 1 commit into from Nov 17, 2015
Merged

Add for_key() and for_value() #8

merged 1 commit into from Nov 17, 2015

Conversation

@dmolesUC
Copy link
Collaborator

dmolesUC commented Nov 17, 2015

See issue #6

@dblock

This comment has been minimized.

Copy link
Owner

dblock commented Nov 17, 2015

I like the implementation, but I don't love the naming. What if we just called these key and value and reversed the behavior? I mean if I want the key from a value I'd call key and if I want the value from the key I'd call value?

This also needs an entry in CHANGELOG and README documentation, please.

@dblock

This comment has been minimized.

Copy link
Owner

dblock commented Nov 17, 2015

Also I think to_h can now be rewritten in terms of one of the memoized variables, but needs to be dup-ed.

dblock referenced this pull request in alterego-labs/ruby-enum Nov 17, 2015
@dblock

This comment has been minimized.

Copy link
Owner

dblock commented Nov 17, 2015

Also see alterego-labs@5c4aff2 which implements what should be key? and value?, both I think are also useful.

@dmolesUC

This comment has been minimized.

Copy link
Collaborator Author

dmolesUC commented Nov 17, 2015

Makes sense. I'll do the renames and (re)implement key? and value?. Do you want me to update to_h in the same pull request?

@dblock

This comment has been minimized.

Copy link
Owner

dblock commented Nov 17, 2015

You should update to_h because it will use your new code, too.

Please don't forget to squash your commits.

@dmolesUC

This comment has been minimized.

Copy link
Collaborator Author

dmolesUC commented Nov 17, 2015

Hmm. As I'm working on additions to the README, I realize I've implemented key? to take a symbol but implemented key and value to return enum instances.

Colors.key?(:RED)
# => true 
Colors.key('red')
# => #<Colors:0x007fbdb49ed620 @key=:RED, @value="red">
Colors.value?('green')
# => true
Colors.value(:GREEN)
# => #<Colors:0x007fbdb49ed4e0 @key=:GREEN, @value="green"> 

Since keys returns symbols and values returns strings, and since AFAICT there's not actually any way to get to the enum instance except with each, this probably isn't the desired behavior. I'll modify key and value to be consistent with keys and values and to_h.

And I'll update to_h and squash the commits.

@dmolesUC

This comment has been minimized.

Copy link
Collaborator Author

dmolesUC commented Nov 17, 2015

Okay, done. I didn't end up updating to_h -- maybe I didn't understand what you were suggesting, but @_enum_hash and @_enums_by_value still both use enum instances, and to_h still needs to extract values, so the existing implementation looks correct to me.

@@ -1,6 +1,6 @@
### Next

* Your contribution here.

This comment has been minimized.

Copy link
@dblock

dblock Nov 17, 2015

Owner

Put this back :)

@@ -1,6 +1,6 @@
### Next

* Your contribution here.
* [#8](https://github.com/dblock/ruby-enum/pull/8): Added `Ruby::Enum#key`, `Ruby::Enum#value`, `Ruby::Enum#key?`, and `Ruby::Enum#value?` - [@dmolesUC3](https://github.com/dmolesUC3)

This comment has been minimized.

Copy link
@dblock

dblock Nov 17, 2015

Owner

Add a period to the end of this sentence.

Nitpicky me ;)

@@ -7,6 +7,17 @@ class Colors
define :GREEN, 'green'
end

# A test class that includes CONSTANT_CASE with underscores

This comment has been minimized.

Copy link
@dblock

dblock Nov 17, 2015

Owner

Can I nitpick on this test? Why do we need X11Colors at all and can't write it all with the existing Colors structure? Less code, more readable, no?

@dblock

This comment has been minimized.

Copy link
Owner

dblock commented Nov 17, 2015

The implementation is 👍, see my other comments? Thanks!

@dmolesUC

This comment has been minimized.

Copy link
Collaborator Author

dmolesUC commented Nov 17, 2015

Sorry, X11Colors is a relic from when I thought there was a bug relating to underscores in keys. I'll take it out and add the contribution line back.

David Moles
@dmolesUC

This comment has been minimized.

Copy link
Collaborator Author

dmolesUC commented Nov 17, 2015

Done.

@dblock

This comment has been minimized.

Copy link
Owner

dblock commented Nov 17, 2015

Perfect, merging.

dblock added a commit that referenced this pull request Nov 17, 2015
Add for_key() and for_value()
@dblock dblock merged commit 4152f72 into dblock:master Nov 17, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dblock dblock mentioned this pull request Nov 19, 2015
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.