-
Notifications
You must be signed in to change notification settings - Fork 61
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
Enum.keys returns strings #100
Comments
Yes, because the internal representation is all strings, can you give an example where this difference is an issue? |
Our code was relying upon Something like this people = [alice, bob, charlie]
User.genders.keys.each do |gender|
these = people.select{|p| p.gender == gender}
do_something with: these
end So to make it work, we'd have to explicitly cast |
You are totally right that simple_enum/lib/simple_enum/enum.rb Line 18 in 2189bbd
The current flow works as follows:
PS: It works similar to HashWithIndifferent access in this regard, by e.g. working with any kind of keys, but internally they are represented as strings Looking back at the code, I really think this should have been designed differently, as the class Hello
as_enum :foo, %w{bar baz}
end
hello = Hello.new(foo: "bar")
hello.foo # => :baz # Why?! Therefore I can see two possible solutions, but not sure which one is "correct"
|
With 1.6 it seems the input type remained intact (strings were strings, symbols were symbols) I guess that 2.x deliberately started returning symbols for everything. If it were me, I'd probably just make sure that things were consistent, |
Sadly, making it consistent, will break the existing API. What I did push for in 2.x was that Symbols is how it should be used, this is also why it returns a Symbol (i.e. that class Syms
as_enum :foo, %i{bar baz}
end
a.foo # => :bar
a.foo = "baz"
a.foo # => :baz
class Strs
as_enum :foo, %w{bar baz}
end
b.foo # => "bar"
b.foo = :baz
b.foo # => "baz" Not sure what is better, honestly... any opinions / voices? |
I see no reason to support both strings and symbols: it's confusing and inconsistent (you don't know what to expect, makes the dev think too much). I think your idea of dropping strings support in favour of symbols is the right one. As @matthewrudy suggested, it might be time for a 3.x :) |
Cool, this would have been my preferred solution too :) anyone willing to make a PR for this? Update: and yep, there's going to be a 3.x release due to semver 👍 |
So @lwe are we agreeing that we should not allow defining enum values as strings anymore? class Hello
as_enum :foo_1, %w{bar baz} # not good
as_enum :foo_2, %i{bar baz} # good
end |
No, I think we agree that the output should always be symbols and it should accept anything. |
To clarify 😄 I think interfaces should accept anything being thrown at them class Hello
as_enum :foo, %w{bar baz} # all right, we can do that
as_enum :boo, "bar" => 1, :baz => 23 # well, if you like that!?
as_enum :zoo, %i{bar baz} # atta boy
end similar to how we accept strings, symbols and indicies as setters: |
Please move discussion to the PR, thus I'm closing this ticket. |
From your example
ie. the enumerated values always appear as symbols everywhere except when calling
keys
.The text was updated successfully, but these errors were encountered: