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

Call undefine_attribute_methods only when defining new attributes #5702

Merged
merged 1 commit into from
May 20, 2012

Conversation

oscardelben
Copy link
Contributor

Every time we call attribute_method_prefix and related methods, we also call undefine_attribute_methods behind the scenes. I think it'd be more clear and possibly more efficient to just do it before we define new attributes.

@drogus
Copy link
Member

drogus commented Apr 2, 2012

I think undefine_attribute_methods should stay there, the purpose of this was probably to allow to call those methods also after define attributes was called.

@oscardelben
Copy link
Contributor Author

The docs specifically say that define_attributes must be after attribute_method_prefix, etc. We super optimize methods definition here, so I thought it didn't make sense to call undefine_attribute_methods every time.

@drogus
Copy link
Member

drogus commented Apr 3, 2012

If the docs say so, I guess it's safe to remove.

/cc @spastorino @josevalim

@steveklabnik
Copy link
Member

This no longer merges cleanly, and will need a rebase.

@oscardelben
Copy link
Contributor Author

Will do

@ghost ghost assigned drogus May 17, 2012
@oscardelben
Copy link
Contributor Author

rebased. cc @drogus

drogus added a commit that referenced this pull request May 20, 2012
Call undefine_attribute_methods only when defining new attributes
@drogus drogus merged commit cae1ca7 into rails:master May 20, 2012
drogus added a commit that referenced this pull request May 20, 2012
This reverts commit cae1ca7, reversing
changes made to da97cf0.

These changes break the build, it needs more investigation.
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.

None yet

4 participants