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

Enum.keys returns strings #100

Closed
matthewrudy opened this issue Apr 27, 2015 · 11 comments · May be fixed by #101
Closed

Enum.keys returns strings #100

matthewrudy opened this issue Apr 27, 2015 · 11 comments · May be fixed by #101
Labels

Comments

@matthewrudy
Copy link

From your example

class User < ActiveRecord::Base
  as_enum :gender, female: 1, male: 0
end

# expected
user.gender == :female
user.genders.key(1) == :female

# but
user.genders.keys == ["male", "female"]

ie. the enumerated values always appear as symbols everywhere except when calling keys.

@lwe
Copy link
Owner

lwe commented Apr 27, 2015

Yes, because the internal representation is all strings, can you give an example where this difference is an issue?

@lwe lwe added the bug label Apr 27, 2015
@matthewrudy
Copy link
Author

Our code was relying upon User.genders.keys.include?(user.gender)

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

@lwe
Copy link
Owner

lwe commented Apr 27, 2015

You are totally right that SimpleEnum::Enum is inconsistent with the return values given and in fact there's only one place where to_sym is used:

key.to_sym if key
- which from an API perspective "feels" wrong too.

The current flow works as follows:

  1. hash as input given via as_enum, e.g. { female: 1, male: 0 }
  2. is passed first to the Hasher to normalize the hash
    def self.map(values, options = {})
    - this basically converts Symbols to Strings and also helps to support items to be stored as strings etc. This method still returns a hash.
  3. resulting hash is passed to SE::Enum and used in there

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 to_sym in SE::Enum feels a bit like a hack to make the comparisons and result values work, also it's weird when the input is a string, it returns magically a Symbol, e.g.

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"

  1. Change API to be consistent and ensure to return appropriate type that was used as input hash -> this will probably break existing code
  2. Change assumption and define that you can throw anything at simple_enum and it always operates under the assumption that Symbols are to be used and returned, because it looks prettier / more ruby-like -> i.e. change SE::Enum#keys to return symbols as well...

@matthewrudy
Copy link
Author

With 1.6 it seems the input type remained intact (strings were strings, symbols were symbols)
So some of my problems today came from these changes in 2.x.

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,
and consider what changes a 3.x release might allow.

@lwe
Copy link
Owner

lwe commented Apr 28, 2015

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 to_sym cast). With the introduction of %i{foo bar} the other option could be to ensure to return whatever was used (e.g. retain 1.x behaviour), assignment would still work with both, strings and symbols, but returned in case of e.g. obj.gender would be the type defined:

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?

@lucke84
Copy link

lucke84 commented Apr 29, 2015

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 :)

@lwe
Copy link
Owner

lwe commented Apr 29, 2015

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 👍

@lucke84
Copy link

lucke84 commented Apr 29, 2015

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

@lwe
Copy link
Owner

lwe commented Apr 29, 2015

No, I think we agree that the output should always be symbols and it should accept anything.

@lwe
Copy link
Owner

lwe commented Apr 29, 2015

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: hello.zoo = "baz" - but we should be strict in what we return / build. So I propose that SE::Enum should only ever expose Symbols as keys to the end user. This means that for all instances of foo, boo and zoo above the keys method should return [:bar, :baz].

@lwe lwe mentioned this issue Apr 29, 2015
3 tasks
@lwe
Copy link
Owner

lwe commented Apr 29, 2015

Please move discussion to the PR, thus I'm closing this ticket.

@lwe lwe closed this as completed Apr 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants