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

Add 'thrice' expectation. #615

Merged
merged 3 commits into from
Aug 6, 2014
Merged

Add 'thrice' expectation. #615

merged 3 commits into from
Aug 6, 2014

Conversation

fimmtiu
Copy link
Contributor

@fimmtiu fimmtiu commented Jul 31, 2014

Added a thrice expectation to complement once and twice. It's a perfectly good English word -- one of my favourites, in fact -- and I was mildly surprised when it didn't work.

Note that I didn't change the output for error messages ("expected foo at least 3 times", etc.), in case anyone's behaviour depends on that.

@yegct
Copy link

yegct commented Jul 31, 2014

"Thrice" appears in the famous poem, Kubla Khan, by Samuel Taylor Coleridge. This alone should justify inclusion. More generally, 'thrice' is an excellent word.

@myronmarston
Copy link
Member

@fimmtiu
Copy link
Contributor Author

fimmtiu commented Jul 31, 2014

Note that the Travis failure appears to be unrelated to this change; it's an error that only occurred in the Ruby 2.1.1 build and which I cannot duplicate, even with the same seed.

@cupakromer
Copy link
Member

I'm not a big fan of this. That said, I can live with it. I've just never been much of a fan of the whole add numeric method names idea, unless it really made a lot of sense. I've seen / used once often enough to see it's merit. twice only rarely, and I can say I've never needed a thrice.

@myronmarston
Copy link
Member

Maybe we should also add forty_thrice and call it "the reddit" :).

@JonRowe
Copy link
Member

JonRowe commented Aug 1, 2014

Like @cupakromer I'm slightly 👎 on this, I don't like wordy additions like this either as it inevitably leads to bloat, there's always just one more, where do you draw the line?

@myronmarston
Copy link
Member

I'm 👍 on this. Given that we have twice, it's reasonable for users to expect thrice.

there's always just one more, where do you draw the line?

I think there's a natural line after thrice because there isn't a word (or at least I don't know it) for "four times" or "five times", etc. Given that thrice is the last commonly known/used word for "x times", I think it makes sense to include it. But as I said, I'd want it to be added to rspec-mocks as well for consistency.

@xaviershay
Copy link
Member

If we're going to include twice we should include thrice. (I don't think we should include either, but w/e.)

@xaviershay
Copy link
Member

If we're going to be consistent, also need yield_thrice_with...

Should add beside twice in built_in_matchers/yield.feature - that's a weird spot for it, but twice isn't docced in features elsewhere.

Needs changelog entry.

@myronmarston
Copy link
Member

If we're going to be consistent, also need yield_thrice_with...

We don't have a yield_once_with or yield_twice_with, so why do we need yield_thrice_with?

@xaviershay
Copy link
Member

I'm sorry, I misread my grep, didn't see it was defined in the feature itself. Move along, nothing to see here...

> ag twice features
features/built_in_matchers/yield.feature
28:        def self.yield_twice_with(*args)
49:        specify { expect { |b| MyClass.yield_twice_with(1, &b) }.to yield_control.twice }
50:        specify { expect { |b| MyClass.yield_twice_with(1, &b) }.to yield_control.exactly(2).times }
51:        specify { expect { |b| MyClass.yield_twice_with(1, &b) }.to yield_control.at_least(1) }
52:        specify { expect { |b| MyClass.yield_twice_with(1, &b) }.to yield_control.at_most(3).times }
58:        specify { expect { |b| MyClass.yield_twice_with(1, &b) }.not_to yield_control.twice }
59:        specify { expect { |b| MyClass.yield_twice_with(1, &b) }.not_to yield_control.at_least(2).times }
60:        specify { expect { |b| MyClass.yield_twice_with(1, &b) }.not_to yield_control.at_least(1) }
61:        specify { expect { |b| MyClass.yield_twice_with(1, &b) }.not_to yield_control.at_most(3).times }
69:      | expected given block not to yield control at least twice  |

@@ -80,7 +80,7 @@ def each_arg(*args, &block)
it 'passes if the block yields the specified number of times' do
expect { |b| [1].each(&b) }.to yield_control.once
expect { |b| [1, 2].each(&b) }.to yield_control.twice
expect { |b| [1, 2, 3].each(&b) }.to yield_control.exactly(3).times
Copy link
Member

Choose a reason for hiding this comment

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

Can you add back an exactly(4).times so this spec covers the same as before?

@JonRowe
Copy link
Member

JonRowe commented Aug 3, 2014

Ok well, just a minor tweak to the specs and I think this is good to go

@myronmarston
Copy link
Member

@fimmtiu are you planning to add thrice to rspec-mocks as well?

fimmtiu added a commit to fimmtiu/rspec-mocks that referenced this pull request Aug 6, 2014
@fimmtiu
Copy link
Contributor Author

fimmtiu commented Aug 6, 2014

@myronmarston I think I've made all the requested changes now.

myronmarston added a commit that referenced this pull request Aug 6, 2014
@myronmarston myronmarston merged commit 44005a5 into rspec:master Aug 6, 2014
@myronmarston
Copy link
Member

Thanks, merged.

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.

6 participants