Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

#should method on CollectionProxy #445

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants

It's a patch for issue #443, but I didn't create tests.
Also since this code duplicates extensions of Kernel module in rspec-core it would be good move methods #should and #should_not to some other module and just include it into Kernel and CollectionProxy.

Something like this:

module RSpec::ShouldMethods
  def should ...
  def should_not ...
end

module Kernel
  include  RSpec::ShouldMethods
end

class ::ActiveRecord::Associations::CollectionProxy
  include  RSpec::ShouldMethods
end
Owner

dchelimsky commented Sep 26, 2011

I'm not sure RSpec should be in the business of trying to out-meta libs that it's spec'ing. If we are going to do something like this, however, I think it should be on BlankSlate. WDYT?

Not rspec, but rspec-rails since it provides a convenient way to test rails applications. There are already some Rails core extentions like #errors_on.
I am not sure Rails developers would be agree to modify there BlankSlate because of it.

Owner

dchelimsky commented Sep 26, 2011

It is implementation-specific extensions like this that forced urgent releases of rspec-rails-1.x with every rails release because the things they depended on changed in rails. This has not been a problem with rspec-rails-2 and I aim to keep it that way. Yes, there are some already, but the fewer, the better.

I was suggesting we implement this on BlankSlate rather than CollectionProxy in order to generalize it and avoid future uses of BlankSlate causing the same problem (not that Rails needs to change). I'm not really comfortable with that, but I'm also uncomfortable with special-casing CollectionProxy.

Also, the idea of extracting the should[_not] methods to a module only to support this case doesn't feel right to me. I'd rather suffer the duplication for now, but revisit if/when a third case emerges.

@myronmarston, @justinko - you guys care to comment?

Owner

myronmarston commented Sep 26, 2011

It is implementation-specific extensions like this that forced urgent releases of rspec-rails-1.x with every rails release because the things they depended on changed in rails. This has not been a problem with rspec-rails-2 and I aim to keep it that way. Yes, there are some already, but the fewer, the better.

I absolutely agree with this general philosophy (and, in fact, I think every library should follow it or something similar). It's always best to depend only upon stable public APIs, and when the goal can't be achieved with existing public APIs, work to get new public APIs added in the upstream libraries that make your goals possible without monkey patching.

That said--I agree this is a pretty major gotcha here and rspec-rails exists largely to smooth out the rough edges between rails and rspec--so I think something should be done to deal with this.

David--I like your suggestions of doing this directly on BlankSlate. Unfortunately, rails doesn't really make this possible. If you look at how CollectionProxy is implemented, it is not a subclass of BlankObject, BlankSlate, or anything similar. It's just blankifying the class inline when it is defined. So there's no good way to do this in a way that'll globally work for all BlankObject/BlankSlate subclasses :(. It would be soooo much easier if rails had an ActiveSupport::BlankSlate class that is simply another name for BasicObject on 1.9 or is their own version of BasicObject on 1.8 since BasicObject is not built in to 1.8.

There's a further problem here: ActiveRecord::Associations::CollectionProxy is a new class in 3.1. It's not there in 3.0. So this solution isn't going to work to fix the issue in 3.0, if it exists there. It looks to me like there is probably the same issue in 3.0, but there the blank-slate class is ActiveRecord::Associations::AssociationProxy (and AssociationCollection is a subclass of that).

Here's one idea that popped in my head for how we might solve this in a more generic/less-version-specific way:

  • Have rspec-expectations use rspec-core's configuration extension API to add a new config option. It could be something like:
RSpec.configure do |c|
  c.add_should_methods_to BlankObject, MyClass
end

Essentially, this provides a general API for adding should and should_not to any class that lacks them.

  • In rspec-rails, have minimal version-specific files that configure this with the proxy classes for the specific versions. For example, there would be a file like active_record_30.rb with:
RSpec.configure do |c|
  c.add_should_methods_to ActiveRecord::Associations::AssociationProxy
end

There could also be an active_record_31.rb file with:

RSpec.configure do |c|
  c.add_should_methods_to ActiveRecord::Associations::CollectionProxy
end
Contributor

justinko commented Sep 27, 2011

add_should_methods_to is nice, but is quite a bit of overhead for something that 99% of codebases will not need to use. Is that a wrong accusation?

I think we might have to go with just special casing AssociationProxy and CollectionProxy - seems like the least amount of footprint.

I am not sure but I guess the issue is not has_many specific. Other associations, for example belongs_to seem to be also proxy objects since they return different object_id than the actual object. So the issue might be reproduced with other associations as well.

Contributor

justinko commented Sep 27, 2011

@greyblake CollectionProxy is the only culprit because it gets "blankified".

@justinko Other are blankified as well. I haven't found it in code, but they behave like proxy objects. However I just realized that other associations do not allow extending.

@myronmarston myronmarston referenced this pull request in rspec/rspec-expectations Mar 4, 2012

Merged

Add support for `expect(value)` syntax. #119

Contributor

justinko commented May 27, 2012

Closing this as the new expect syntax as been merged.

@justinko justinko closed this May 27, 2012

Owner

dchelimsky commented Aug 3, 2012

Now that we have an easy way to add should and should_not to any arbitrary class (thanks @myronmarston) we're adding this.

@dchelimsky dchelimsky reopened this Aug 3, 2012

Owner

dchelimsky commented Aug 3, 2012

Although we're using what @myronmarston built into rspec-expectations. Thanks @greyblake for the pull request, but this didn't exist when you made the pull request and I think it's the right choice now that it does.

@dchelimsky dchelimsky closed this in abfd0fc Aug 3, 2012

@dchelimsky dchelimsky reopened this Aug 5, 2012

Owner

dchelimsky commented Aug 5, 2012

Sorry - gotta reopen this. What I implemented forces rspec-expectations on all rspec-rails users.

This pull request fails (merged b39c778 into 9805468).

Owner

myronmarston commented Aug 6, 2012

@dchelimsky -- I just pushed a fix an improved version of this that does not force rspec-expecations on all users and works with Rails 3.0.x. Let me know if you have any concerns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment