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

Delegate CollectionDecorator#kind_of? to the underlying decorated collection #497

Merged
merged 2 commits into from
Mar 11, 2013
Merged

Delegate CollectionDecorator#kind_of? to the underlying decorated collection #497

merged 2 commits into from
Mar 11, 2013

Conversation

haines
Copy link
Contributor

@haines haines commented Mar 11, 2013

Incorporates #488, using alias_method instead of alias for consistency. Thanks, @jtrim!

Addresses a regression previously addressed in #92. The regression directly affects how nested forms are rendered for has_many associations decorated with 'decorates_associations'.

@jtrim
Copy link
Contributor

jtrim commented Mar 11, 2013

Thanks for picking this up! However, I think maybe what you intended to do was something along the lines of:

alias_method :orig_kind_of, :kind_of
def kind_of?(klass)
  decorated_collection.kind_of?(klass) || orig_kind_of(klass)
end
alias :is_a? :kind_of?

The line that you replaced in 63721db is necessary as long as we want to maintain the is_a? api likeness to kind_of?. If this is the case, I'm not sure I see the benefit of using alias_method to define an extra instance method as opposed to calling through to super. I could very well be missing something though.

@steveklabnik
Copy link
Member

The JRuby failures are Travis' fault, so I'm not worrying about that. It's fixed in edge bundler/rubygems, they should be releasing/updating soonish.

I think that this is fine, both of them work, but this way, we don't have the extra method lying around.

steveklabnik added a commit that referenced this pull request Mar 11, 2013
Delegate CollectionDecorator#kind_of? to the underlying decorated collection
@steveklabnik steveklabnik merged commit d5975f2 into drapergem:master Mar 11, 2013
@haines haines deleted the collection-decorator-kind-of branch March 11, 2013 19:08
@haines
Copy link
Contributor Author

haines commented Mar 11, 2013

@steveklabnik 👍

@jtrim alias and alias_method are pretty much equivalent, and we have tended to use alias_method throughout so I only changed it for consistency's sake... it does exactly the same thing as your version.

@jtrim
Copy link
Contributor

jtrim commented Mar 11, 2013

Yeah, I tend to prefer plain alias because of the internal Ruby optimizations it provides, but in terms of behavior you're right on equivalency. Sorry about my previous comment - I saw the build failure and jumped to a conclusion about alias_method usage. Thanks for pulling this in!

Alexander-Senko added a commit to Alexander-Senko/draper that referenced this pull request Sep 12, 2024
Decorators shouldn't pretend to be instances of the underlying model 
classes for the following reasons:

1. It's terribly wrong to pretend being a class without implementing 
it's interface. It can break third party code.
2. Hacking Ruby core methods can lead to unexpected and hard to 
understand behavior. It should be avoided without a strong reason 
behind.
3. I'd prefer compatibility patches to be narrow-scoped and live in 
there own modules.

Resolves drapergem#859.
Reverts  drapergem#72,
         drapergem#110,
         drapergem#417,
         drapergem#497.
Alexander-Senko added a commit that referenced this pull request Sep 13, 2024
Decorators shouldn't pretend to be instances of the underlying model 
classes for the following reasons:

1. It's terribly wrong to pretend being a class without implementing 
it's interface. It can break third party code.
2. Hacking Ruby core methods can lead to unexpected and hard to 
understand behavior. It should be avoided without a strong reason 
behind.
3. I'd prefer compatibility patches to be narrow-scoped and live in 
there own modules.

Resolves #859.
Reverts  #72,
         #110,
         #417,
         #497.
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.

3 participants