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

Extract primary_key to AbstractReflection #30208

Merged

Conversation

kamipo
Copy link
Member

@kamipo kamipo commented Aug 12, 2017

No description provided.

@matthewd
Copy link
Member

Hmm.. I'm not sure what I think of this. On one hand, it avoids a duplicated method... but on the other, it adds that method to a bunch of classes for which it doesn't make sense. 🤔

@kamipo
Copy link
Member Author

kamipo commented Aug 12, 2017

Should create new PR?

@kamipo kamipo force-pushed the extract_primary_key_to_abstract_reflection branch from 59bcd2c to 0e508e2 Compare August 12, 2017 11:25
@kamipo
Copy link
Member Author

kamipo commented Aug 12, 2017

I've created new PR #30209 and removed the commit that remove duplicated method in this PR.

@rafaelfranca
Copy link
Member

I think if the method doesn't make sense in other classes we should not add it to the superclass even if it removed duplication. Unless it is a private utility method. If it is public, better to keep the duplication, or extract it to a module. In this case, the duplication is just fine in my opinion.

@kamipo
Copy link
Member Author

kamipo commented Aug 13, 2017

Actually the primary_key is a private utility method that is used in association_primary_key and active_record_primary_key (by the way, association_primary_key has a bug #27609).
So I placed the method at bottom of private methods in AbstractReflection.

@rafaelfranca rafaelfranca merged commit 2ffb12d into rails:master Aug 14, 2017
@kamipo kamipo deleted the extract_primary_key_to_abstract_reflection branch August 15, 2017 02:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants