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

Documentation and cleanup of automatic discovery of inverse associations #10886

Merged

Conversation

@wangjohn
Copy link
Contributor

@wangjohn wangjohn commented Jun 8, 2013

Removed :automatic_inverse_of in favor of using the inverse_of: false to tell us whether we don't want to use automatic inverses. Changing the documentation and adding a CHANGELOG entry for the automatic inverse detection feature.

@jonleighton I don't think it's actually possible to use inverse_of: nil to tell us to not automatically find inverse associations. This is because the feature would be completely useless, since by default, we have options[:inverse_of] = nil. This means, by default we do not use automatic inverse finding.

Conversely, having the user specify options[:inverse_of] = true to use automatic associations doesn't make sense because at that point, the user can make the extra couple characters and type in the inverse.

@robin850
robin850 reviewed Jun 8, 2013
View changes
activerecord/CHANGELOG.md Outdated
especially the ones with non-standard names.

You can turn off the automatic detection of inverse associations by setting
the <tt>:inverse_of</tt> option to <tt>false</tt> like so:

This comment has been minimized.

@robin850

robin850 Jun 8, 2013
Member

Why do you use <tt> elements there instead of backticks just like before? In my opinion, it's more readable with backticks. 😄

This comment has been minimized.

@jonleighton

jonleighton Jun 8, 2013
Member

Yeah, this should be backticks because it's a Markdown file.

@jonleighton
Copy link
Member

@jonleighton jonleighton commented Jun 8, 2013

@wangjohn regarding false vs nil, there's a difference between not having an :inverse_of option, and having a nil option. i.e. you could do options.include?(:inverse_of) && options[:inverse_of].nil?. Anyway, I don't really mind it being false actually, so let's leave it.

@wangjohn
Copy link
Contributor Author

@wangjohn wangjohn commented Jun 8, 2013

@jonleighton Ahh, thanks for the pointer, that does make sense.

@robin850 Thank you for the comment, I've fixed the CHANGELOG entry to use backticks instead.

…ns in favor

of using +inverse_of: false+ option. Changing the documentation and
adding a CHANGELOG entry for the automatic inverse detection feature.
jonleighton added a commit that referenced this pull request Jun 9, 2013
…associations

Documentation and cleanup of automatic discovery of inverse associations
@jonleighton jonleighton merged commit ae6e6d9 into rails:master Jun 9, 2013
#
# Anything with a scope can additionally ruin our attempt at finding an
# inverse, so we exclude reflections with scopes.
def can_find_inverse_of_automatically?(reflection)
reflection.options[:automatic_inverse_of] != false &&
reflection.options[:inverse_of] != false &&

This comment has been minimized.

@egilburg

egilburg Jun 9, 2013
Contributor

You can do reflection.options.keys.include?(:inverse_of) && !reflection.options[:inverse_of] so passing either explicit nil or false would work. I agree with original review that nil is more intuitive since it makes the line semantics more declarative (e.g. as a Rails developer I say there is 'no' defined inverse, rather than explicitly saying 'do not run some process that sets it'.

This comment has been minimized.

@Victorcorcos

Victorcorcos Aug 15, 2018

@egilburg Very interesting line of reasoning.

@lassebunk
Copy link

@lassebunk lassebunk commented Dec 20, 2013

@egilburg 👍

@duduribeiro
Copy link
Contributor

@duduribeiro duduribeiro commented Jan 22, 2015

👍

@cyrilchampier
Copy link

@cyrilchampier cyrilchampier commented Apr 10, 2018

sorry if this is not the right place to ask, but what would be a good reason to disable this behavior ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.