Remove have(n).items matchers #293

Merged
merged 1 commit into from Aug 17, 2013

Conversation

Projects
None yet
6 participants
@hugobarauna
Member

hugobarauna commented Jul 21, 2013

As explained in the plan for RSpec 3, the have(n).items matchers are going to be extracted to an external gem. So, this PR removes those matchers from rspec-expectations.

Remove have(n).items matchers
Those mastchers will be maintained by Hugo Barauna in a external
gem.
@@ -92,21 +92,6 @@ def object.has_taste_for?(*args); true; end
expect(RSpec::Matchers.generated_description).to eq 'should have taste for "wine", "cheese"'
end
- it "expect(...).to have n items" do

This comment has been minimized.

@hugobarauna

hugobarauna Jul 21, 2013

Member

@myronmarston These 3 specs are related to the have(n).items matchers. Although I'm removing them from rspec-expectations, I'm not sure if I should move them to the new rspec-collection_matchers gem. Should I move them to the new repo?

@hugobarauna

hugobarauna Jul 21, 2013

Member

@myronmarston These 3 specs are related to the have(n).items matchers. Although I'm removing them from rspec-expectations, I'm not sure if I should move them to the new rspec-collection_matchers gem. Should I move them to the new repo?

This comment has been minimized.

@JonRowe

JonRowe Jul 21, 2013

Member

Yes because you still want the generated descriptions.

@JonRowe

JonRowe Jul 21, 2013

Member

Yes because you still want the generated descriptions.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jul 21, 2013

Coverage Status

Coverage decreased (-0%) when pulling 8f39d44 on hugobarauna:remove-have-n-items-matchers into b43952f on rspec:master.

Coverage Status

Coverage decreased (-0%) when pulling 8f39d44 on hugobarauna:remove-have-n-items-matchers into b43952f on rspec:master.

@samphippen

This comment has been minimized.

Show comment
Hide comment
@samphippen

samphippen Jul 21, 2013

Member

@hugobarauna I wouldn't feel comfortable merging this until we've got the external gem. Also what about rspec-collection-matchers instead of rspec-collection_matchers?

Member

samphippen commented Jul 21, 2013

@hugobarauna I wouldn't feel comfortable merging this until we've got the external gem. Also what about rspec-collection-matchers instead of rspec-collection_matchers?

@alindeman

This comment has been minimized.

Show comment
Hide comment
@alindeman

alindeman Jul 21, 2013

Contributor

I think rspec-collection_matchers is the "right" way here if there are two words in the gem extension.

See: http://guides.rubygems.org/patterns/#consistent-naming and as an example: https://github.com/rails/activerecord-session_store

Contributor

alindeman commented Jul 21, 2013

I think rspec-collection_matchers is the "right" way here if there are two words in the gem extension.

See: http://guides.rubygems.org/patterns/#consistent-naming and as an example: https://github.com/rails/activerecord-session_store

@samphippen

This comment has been minimized.

Show comment
Hide comment
@samphippen

samphippen Jul 21, 2013

Member

@alindeman that link makes me think it should be named like this. But it still makes me sad.

Member

samphippen commented Jul 21, 2013

@alindeman that link makes me think it should be named like this. But it still makes me sad.

@hugobarauna

This comment has been minimized.

Show comment
Hide comment
@hugobarauna

hugobarauna Jul 21, 2013

Member

@samphippen I agree with you, we should merge this PR only after having the external gem. I already created a local git repo for that gem in my machine, meanwhile, I already talked to @myronmarston and he's setting up a repo for that gem to live in.

After the he set this up, I'll send the code to that github repo and I'll ask for some code review from you guys. 😉

Member

hugobarauna commented Jul 21, 2013

@samphippen I agree with you, we should merge this PR only after having the external gem. I already created a local git repo for that gem in my machine, meanwhile, I already talked to @myronmarston and he's setting up a repo for that gem to live in.

After the he set this up, I'll send the code to that github repo and I'll ask for some code review from you guys. 😉

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Jul 22, 2013

Member

Thanks for starting this. I've been busy this weekend (we just bought a house and are doing lots of painting and packing in preparation to move next week), but I'll try to make time to make the repo in the next day or so.

Member

myronmarston commented Jul 22, 2013

Thanks for starting this. I've been busy this weekend (we just bought a house and are doing lots of painting and packing in preparation to move next week), but I'll try to make time to make the repo in the next day or so.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Jul 22, 2013

Member

@hugobarauna -- the repo is setup for you now:

https://github.com/rspec/rspec-collection_matchers

Have at it!

Member

myronmarston commented Jul 22, 2013

@hugobarauna -- the repo is setup for you now:

https://github.com/rspec/rspec-collection_matchers

Have at it!

@hugobarauna hugobarauna referenced this pull request in rspec/rspec-collection_matchers Jul 23, 2013

Merged

Bring the have(n).items matcher from rspec-expectations #1

@samphippen

This comment has been minimized.

Show comment
Hide comment
@samphippen

samphippen Aug 3, 2013

Member

Can this be closed now?

Member

samphippen commented Aug 3, 2013

Can this be closed now?

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Aug 3, 2013

Member

I think it needs to be merged with the gem is ready? Same situation as autotest.

Member

JonRowe commented Aug 3, 2013

I think it needs to be merged with the gem is ready? Same situation as autotest.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Aug 5, 2013

Member

I think it needs to be merged with the gem is ready? Same situation as autotest.

"ready" is such a generic goal, though. Software is never done and is always changing.

I'd say I'm OK merging this and the autotest PR once the new gem is to a place where we're 100% sure that the removal is going to work out OK. Are there any lingering issues for either?

If not, we can go ahead and merge these.

Member

myronmarston commented Aug 5, 2013

I think it needs to be merged with the gem is ready? Same situation as autotest.

"ready" is such a generic goal, though. Software is never done and is always changing.

I'd say I'm OK merging this and the autotest PR once the new gem is to a place where we're 100% sure that the removal is going to work out OK. Are there any lingering issues for either?

If not, we can go ahead and merge these.

@hugobarauna

This comment has been minimized.

Show comment
Hide comment
@hugobarauna

hugobarauna Aug 5, 2013

Member

I already finished the first PR in https://github.com/rspec/rspec-collection_matchers and the have(n) matchers are already in its master branch. But, although I've finished the PR, there's at least 2 must have that I still want to finish to call it a 0.1 version:

  1. make CI tests green. They're currently breaking for 1 ruby version, which is jruby 1.8
  2. Write the README
Member

hugobarauna commented Aug 5, 2013

I already finished the first PR in https://github.com/rspec/rspec-collection_matchers and the have(n) matchers are already in its master branch. But, although I've finished the PR, there's at least 2 must have that I still want to finish to call it a 0.1 version:

  1. make CI tests green. They're currently breaking for 1 ruby version, which is jruby 1.8
  2. Write the README
@JonRowe

This comment has been minimized.

Show comment
Hide comment
Member

JonRowe commented Aug 6, 2013

See rspec/rspec-collection_matchers#3 for JRuby fix ;)

myronmarston added a commit that referenced this pull request Aug 17, 2013

@myronmarston myronmarston merged commit 4ccacb8 into rspec:master Aug 17, 2013

1 check passed

default The Travis CI build passed
Details

@hugobarauna hugobarauna deleted the hugobarauna:remove-have-n-items-matchers branch Aug 18, 2013

@soulcutter soulcutter referenced this pull request in rspec/rspec-rails Aug 19, 2013

Closed

Extract `have(n).records` support #808

@yujinakayama yujinakayama referenced this pull request in yujinakayama/transpec Sep 23, 2013

Closed

Support conversion of collection cardinality matchers #5

@myronmarston myronmarston referenced this pull request in rspec/rspec-core Sep 23, 2013

Closed

Extract `its` into external `rspec-its` gem #1083

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