Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Refactored class_attribute. #8435

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
5 participants

jwaldrip commented Dec 6, 2012

class_attribute now uses ruby instance vars rather than redefining the value as a method. Also added a new option allowing for the value to not be persisted on inheritance, that option defaults to true (does persist).

jwaldrip added some commits Dec 6, 2012

@jwaldrip jwaldrip Refactored Class#class_atrribute
class_attribute now uses ruby instance vars rather than redefining the value as a method. Also added a new option allowing for the value to not be persisted on inheritance, that option defaults to true (does persist).
6ea3331
@jwaldrip jwaldrip class_attribute changes 848127e
@jwaldrip jwaldrip API Documentation for persist_when_inherited option. 131ad19

Thanks @jwaldrip, but I don't see a use case where I'd not like the inherited class to have the same attr value as the parent one. That's basically the proposal of class_attribute in my mind. At the last resort, if I really wanted to have that functionality, I think I'd rather use Class attr accessors: class << self; attr_accessor :foo; end.

About the refactoring, I think the implementation uses class_eval because it's faster than define_method.

Lets get some more feedback first. Thanks for your contribution!

/cc @josevalim @rafaelfranca

Owner

pixeltrix commented Dec 6, 2012

In fact such a change was reverted in 8601f73

Contributor

thedarkone commented Dec 6, 2012

This is a bad idea, the original code is written this way (raw @ivar access, class_eval etc.) for performance reasons.

Owner

pixeltrix commented Dec 6, 2012

I think we can close this as it's accepted that it's being done this way for performance reasons - thanks for you input anyway @jwaldrip.

@pixeltrix pixeltrix closed this Dec 6, 2012

Owner

fxn commented Dec 6, 2012

If the intention of the implementation is key but not obvious to the reader, maybe the file could have some internal comment about it?

Sounds like a good plan to me.

On Thu, Dec 6, 2012 at 10:33 AM, Xavier Noria notifications@github.comwrote:

If the intention of the implementation is key but not obvious to the
reader, maybe the file could have some internal comment about it?


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/pull/8435#issuecomment-11083554.

At.
Carlos Antonio

@pixeltrix pixeltrix added a commit that referenced this pull request Dec 6, 2012

@pixeltrix pixeltrix Add comment about implementation of class_attribute
To prevent future pull requests like #8435 add a comment about the
implementation of class_attribute using class_eval for performance.

[ci skip]
8942035

jwaldrip commented Dec 7, 2012

I completely understand the need for class_eval, but is there a performance decrease by just using the instance variable method within a class eval?

Contributor

thedarkone commented Dec 7, 2012

I completely understand the need for class_eval, but is there a performance decrease by just using the instance variable method within a class eval?

Yes, if it replaces direct @ivar access. I also find

def foo
  @foo
end

to be more readable than:

def foo
  instance_variable_get(:@foo)
end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment