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

Pretend to be instance_of?(source.class) #417

Merged
merged 2 commits into from
Jan 13, 2013
Merged

Pretend to be instance_of?(source.class) #417

merged 2 commits into from
Jan 13, 2013

Conversation

haines
Copy link
Contributor

@haines haines commented Jan 12, 2013

We pretend to be a kind_of? the source class, so I think it also makes sense to pretend to be an instance_of? the same.

I only had to tweak the handle_multiple_decoration stuff, otherwise no changes were necessary (although the specs will fail on Travis until #416 goes in).

As a nice bonus, this would make this spec pass on Rails 3.0 - it doesn't at the moment because in 3.0 ActiveRecord::Base#== doesn't call super, so it doesn't hit Decoratable#==:

# activerecord/lib/active_record/base.rb#L1629
def ==(comparison_object)
  comparison_object.equal?(self) ||
    (comparison_object.instance_of?(self.class) &&
      comparison_object.id == id && !comparison_object.new_record?)
end

Compared to, on 3.2 (and similarly on 3.1),

# activerecord/lib/active_record/base.rb#L593
def ==(comparison_object)
  super ||
    comparison_object.instance_of?(self.class) &&
    id.present? &&
    comparison_object.id == id
end

@steveklabnik
Copy link
Member

This is also super failing on Travis.

@haines
Copy link
Contributor Author

haines commented Jan 13, 2013

Yeah, this is going to fail hard at the moment due to the minitest-rails update, but it'll be fine after #416.

@steveklabnik
Copy link
Member

Re-building!

@haines
Copy link
Contributor Author

haines commented Jan 13, 2013

Travis seems to be doing something slightly odd with it. I'll rebase it and see what happens!

@haines
Copy link
Contributor Author

haines commented Jan 13, 2013

Hmmm, we had another round of that internal server error on an earlier build (before I fixed a goof that was giving a spurious warning, and removed the pending spec).

Since it's almost certainly unrelated to this particular PR, I think this can still go in, but I will investigate further.

steveklabnik added a commit that referenced this pull request Jan 13, 2013
Pretend to be instance_of?(source.class)
@steveklabnik steveklabnik merged commit 740b32e into drapergem:master Jan 13, 2013
@steveklabnik
Copy link
Member

Seems legit.

@haines haines deleted the instance_of branch January 13, 2013 15:39
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.

2 participants