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

ID as symbols. #28

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

ID as symbols. #28

wants to merge 5 commits into from

Conversation

hackling
Copy link

Add functionality to active_enum to be able to use symbols as IDs.

Example usage

class Sex < ActiveEnum::Base
  value :id => :m, :name => 'Male'
  value :id => :f, :name => 'Female'
end

@kuraga
Copy link
Contributor

kuraga commented Jun 14, 2013

Thanks very much! See #20 - it was my dream... And what about Strings, what do you think?

@hackling
Copy link
Author

The reason this PR came about was because we were initially using strings as IDs and it was causing all sorts of issues.

While I think that it can be done, after some discussion, we decided that it was best not to implement as it, quickly becomes confusing as to what is what, and the get and meta functions would need more of a rework.

Using IDs as symbols and names as strings, it's far easier to differentiate between the two.

@kuraga
Copy link
Contributor

kuraga commented Jun 14, 2013

Thanks very much for deep explanation!

@gsmetal
Copy link

gsmetal commented Jul 9, 2013

I think that attribute setter is not working as expected. This code from extensions.rb

def define_active_enum_write_method(attribute)
  class_eval <<-DEF
     def #{attribute}=(arg)
       if arg.is_a?(Symbol)
         super self.class.active_enum_for(:#{attribute})[arg]
       else
         super arg
       end
     end
  DEF
end

causes saving name value to database, not id.

@gsmetal
Copy link

gsmetal commented Jul 9, 2013

And same for getter: string loaded from database, so it doesn't find value in storage.

@hackling
Copy link
Author

hackling commented Jul 9, 2013

Just got back from a month holiday. Will have a look at this once I've been back behind the keyboard for a while and in a proper frame of mind.

No doubt the test coverage will need to be expanded to cover this.

@gsmetal
Copy link

gsmetal commented Jul 24, 2013

I'm sorry, but have you looked at this? Is it hard to correct?

@hackling
Copy link
Author

I haven't had a chance yet. Had a few major deadlines lately, however this is on the list of things to fix and review as it's come up in development of one of our apps.

@felixbuenemann
Copy link
Contributor

I also have the need for string ids. For now I solved it by forking active_enum and replacing all the checks for is_a?(Fixnum)with is_a?(Fixnum) || is_a?(String), see felixbuenemann/active_enum@a2bc720.
So I'm using the opposite logic to what's proposed here.

I'm not yet sure as to what's better: Enforcing symbols or strings for IDs. In any case it breaks some behaviour of the bracket/get accessor.

@gsmetal gsmetal mentioned this pull request Feb 28, 2014
@gsmetal
Copy link

gsmetal commented Feb 28, 2014

Finally, I corrected some problems and added tests in my PR: #38

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.

6 participants