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

"loaded_#{association}?" method removed from belongs_to association #472

Closed
aaronchi opened this Issue May 9, 2011 · 6 comments

Comments

Projects
None yet
4 participants
@aaronchi
Contributor

aaronchi commented May 9, 2011

There used to be a method on models with belongs_to associations that would tell you whether the association is loaded. This still works on has_many associations with collection.loaded? but no longer works for belongs_to

@ghost ghost assigned tobi and tenderlove May 9, 2011

@jonleighton

This comment has been minimized.

Show comment
Hide comment
@jonleighton

jonleighton May 10, 2011

Member

This was never a documented public method, but I guess we should decide whether it should exist as such.

@tenderlove, what do you think?

In the meantime you can do foo.association(:bla).loaded? to check, but if we are going to have a public API for this then it should probably be bla_loaded?.

Member

jonleighton commented May 10, 2011

This was never a documented public method, but I guess we should decide whether it should exist as such.

@tenderlove, what do you think?

In the meantime you can do foo.association(:bla).loaded? to check, but if we are going to have a public API for this then it should probably be bla_loaded?.

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove May 10, 2011

Member

@jonleighton Really? bla_loaded? ? I don't like it since that means more method missing or dynamically added methods. If we can't add it to the association proxy, the foo.association(:bla).loaded? form seems fine to me. I don't see this as something people commonly do, so asking the association directly seems fine. wdyt?

Member

tenderlove commented May 10, 2011

@jonleighton Really? bla_loaded? ? I don't like it since that means more method missing or dynamically added methods. If we can't add it to the association proxy, the foo.association(:bla).loaded? form seems fine to me. I don't see this as something people commonly do, so asking the association directly seems fine. wdyt?

@jonleighton

This comment has been minimized.

Show comment
Hide comment
@jonleighton

jonleighton May 10, 2011

Member

@tenderlove: it's not method_missing as we can generate the method when the association is defined, but I agree with you that it's not great to be dynamically generating loads of methods.

However, my concern with foo.association(:bla).loaded? is that if we supported this as a public API then it exposes the whole foo.association(:bla) 'area' which we might prefer to keep under the covers. (We don't want people complaining about us removing other undocumented methods from foo.association(:bla) in the future.)

My preference is to just preserve the status quo: people can use foo.association(:bla).loaded? if they must, and it's unlikely to change, but they mustn't hate if it does change ;)

Member

jonleighton commented May 10, 2011

@tenderlove: it's not method_missing as we can generate the method when the association is defined, but I agree with you that it's not great to be dynamically generating loads of methods.

However, my concern with foo.association(:bla).loaded? is that if we supported this as a public API then it exposes the whole foo.association(:bla) 'area' which we might prefer to keep under the covers. (We don't want people complaining about us removing other undocumented methods from foo.association(:bla) in the future.)

My preference is to just preserve the status quo: people can use foo.association(:bla).loaded? if they must, and it's unlikely to change, but they mustn't hate if it does change ;)

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove May 10, 2011

Member

TBH, I don't see a problem with exposing the association objects. Presumably we're making private methods private, and public methods public.

I agree. Keep status quo. :-)

Member

tenderlove commented May 10, 2011

TBH, I don't see a problem with exposing the association objects. Presumably we're making private methods private, and public methods public.

I agree. Keep status quo. :-)

@jonleighton

This comment has been minimized.

Show comment
Hide comment
@jonleighton

jonleighton May 10, 2011

Member

Okay, closing this then.

Member

jonleighton commented May 10, 2011

Okay, closing this then.

@jonleighton jonleighton reopened this May 11, 2011

@jonleighton

This comment has been minimized.

Show comment
Hide comment
@jonleighton

jonleighton May 11, 2011

Member

I felt bad about this when I woke up this morning, so I am going to add a deprecation warning.

Member

jonleighton commented May 11, 2011

I felt bad about this when I woke up this morning, so I am going to add a deprecation warning.

@ghost ghost assigned jonleighton May 11, 2011

matthewd added a commit that referenced this issue Apr 24, 2018

Merge pull request #472 from film42/master
Make Visitor visit thread safe by holding dispatch method reference
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment