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

make changed_attributes work with new enum feature #13489

Closed
wants to merge 1 commit into from

Conversation

barelyknown
Copy link

Changed per the discussion in PR #13267.

@@ -180,7 +180,6 @@ def attribute_change(attr)
# Handle <tt>*_will_change!</tt> for +method_missing+.
def attribute_will_change!(attr)
return if attribute_changed?(attr)

Copy link
Member

Choose a reason for hiding this comment

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

Can you add this back?

@chancancode
Copy link
Member

I'm not sure if doing this from the read_attribute level is the right approach. (And since this is enum-specific, if we were to do this, we should probably be overriding read_attribute and call super.) Masking access to the raw DB value for this purpose seems like a bad idea (and the fact that write_attribute doesn't work the same would be quite odd as well). I view Enums as a higher-level connivence feature that builds on top of the lower level AR primitives, so we should probably deal with this from a higher level.

(Sorry - I haven't looked into this enough to suggest a good alternative approach yet, just adding my concerns for now)

@chancancode
Copy link
Member

cc @senny @dhh

@barelyknown
Copy link
Author

I agree that it's better to override read_attribute and call super. I made that adjustment and the code is much better. While I'm not 100% confident that changing read_attribute is the right strategy, it seemed like the best place as I read through the code. The fact that the enum is stored as an integer is a performance issue in my opinion, and I couldn't think of why the user would ever want to deal with the integer when using an ActiveRecord instance. So, self[enum_attribute_name] should also return the string version not the integer I think.

@barelyknown
Copy link
Author

What does everyone think of this? It'd great to
have this in 4.1.


def enum_attribute?(attr_name)
klass = self
return false unless defined?(klass::ENUMS)
Copy link
Contributor

Choose a reason for hiding this comment

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

why can't just use self::ENUMS here and in other places using klass?

Copy link
Author

Choose a reason for hiding this comment

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

I cargo culted from the enum method right below. I wanted to follow the convention that the rest of the module was using.

@ghost ghost assigned chancancode Jan 9, 2014
end
end

private
def _enum_methods_module
@_enum_methods_module ||= begin
mod = Module.new
mod = Module.new do

Copy link
Member

Choose a reason for hiding this comment

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

Kill newlines after block indentions here and anywhere else as well.

@dhh
Copy link
Member

dhh commented Jan 13, 2014

Would this impact the solution we had to dealing with form fields and enums?

def enum(definitions)
klass = self
klass.const_set("ENUMS", ActiveSupport::HashWithIndifferentAccess.new) unless defined?(klass::ENUMS)
Copy link
Member

Choose a reason for hiding this comment

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

Let's leave this (and the related code) out of this PR, I'll tackle that in #13450

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 this pull request may close these issues.

6 participants