Skip to content
This repository was archived by the owner on Nov 30, 2024. It is now read-only.

Conversation

alindeman
Copy link
Contributor

  • Hopefully the code clarity is improved here
  • We also change the behavior by not duping an object if space is
    not yet initialized. This should have been the behavior from the
    beginning, but it was overlooked and did not have specs.

* Hopefully the code clarity is improved here
* We also change the behavior by *not* duping an object if `space` is
  not yet initialized. This should have been the behavior from the
  beginning, but it was overlooked and did not have specs.
@JonRowe
Copy link
Member

JonRowe commented Jul 1, 2013

I like it...

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 77b64fe on alindeman:issue-950-with-specs into 65745a0 on rspec:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 77b64fe on alindeman:issue-950-with-specs into 65745a0 on rspec:master.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't a merge blocker by any means, but I think if ::RSpec::Mocks.space.nil? is a bit clearer than if !::RSpec::Mocks.space. Not worth taking extra time to fixup, though, unless you wind up doing more work here for some other reason.

@myronmarston
Copy link
Member

👍 Much better. Did you confirm your change fixes the spork issue? I expect it does (given that we now know that the old code would unnecessarily dup), but figured it was worth asking.

@alindeman
Copy link
Contributor Author

Pushed new commits based on your comments.

👍 Much better. Did you confirm your change fixes the spork issue? I expect it does (given that we now know that the old code would unnecessarily dup), but figured it was worth asking.

Yep! I used your recurring_select repo. It's blowing up with TypeError on master, but works correctly on this branch.

Ready for re-review :)

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling ed527b9 on alindeman:issue-950-with-specs into 65745a0 on rspec:master.

@myronmarston
Copy link
Member

LGTM. I guess this just needs a changelog and to be merged into the appropriate branches.

@JonRowe
Copy link
Member

JonRowe commented Jul 2, 2013

❤️

@alindeman alindeman merged commit bcee056 into rspec:master Jul 2, 2013
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling bcee056 on alindeman:issue-950-with-specs into 65745a0 on rspec:master.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants