Change have(n).things to have(n, :things) #93

Closed
dchelimsky opened this Issue Oct 26, 2011 · 21 comments

Comments

Projects
None yet
10 participants
@dchelimsky
Owner

dchelimsky commented Oct 26, 2011

Per https://gist.github.com/893459, the have(n).whatever api can be confusing since whatever is handled by method_missing, so there is no clear place to search in the code or even rdoc to understand whatever is.

Proposal: change have(n).whatever to have(n, :whatever). This reads out loud exactly the same way, and a user would who wanted to understand what :whatever would likely look to the right place in the rdoc to do so.

We can do this by presenting deprecation warnings from method_missing when used in this way for 2.x, and then removing support entirely in 3.0.

@rgarner

This comment has been minimized.

Show comment Hide comment
@rgarner

rgarner Oct 26, 2011

It only has to read in the same order, for me, which this does. Is there any reason why the deprecation for the method_missing way has to take place at all? Newcomers would use the canonical method from the RDoc, old hands will continue to use muscle memory. Is there a downside to having both post-3.0?

rgarner commented Oct 26, 2011

It only has to read in the same order, for me, which this does. Is there any reason why the deprecation for the method_missing way has to take place at all? Newcomers would use the canonical method from the RDoc, old hands will continue to use muscle memory. Is there a downside to having both post-3.0?

@dchelimsky

This comment has been minimized.

Show comment Hide comment
@dchelimsky

dchelimsky Oct 26, 2011

Owner

The problem described in the gist is that users get confused by the method. Without removing support for that, the problem would still exist, even if diminished by this new alternative. I'd consider postponing the deprecation removal to 3.0/4.0 respectively, but I don't want rspec to be bloated supporting older APIs.

Owner

dchelimsky commented Oct 26, 2011

The problem described in the gist is that users get confused by the method. Without removing support for that, the problem would still exist, even if diminished by this new alternative. I'd consider postponing the deprecation removal to 3.0/4.0 respectively, but I don't want rspec to be bloated supporting older APIs.

@justinko

This comment has been minimized.

Show comment Hide comment
@justinko

justinko Oct 26, 2011

Contributor

+1

Contributor

justinko commented Oct 26, 2011

+1

@gma

This comment has been minimized.

Show comment Hide comment
@gma

gma Nov 10, 2011

I appreciate the effort to improve it after my slightly ranty gist, but I think the proposed change misses the mark slightly.

Good programmers are inquisitive. If they see something they don't understand the need for, they investigate. They're driven by a need to understand the code in front of them. If you show a good programmer some code with a seemingly unnecessary parameter or method call in it, they'll say "hang on, whats going on here?" and take a diversion from what they were meant to be doing in order to read the source. If that parameter/method serves no actual purpose you've just wasted the good programmer's time and energy.

New and inquisitive programmers who are destined to become good programmers are afflicted with the same curiousity.

Good APIs are self explanatory.

When you consider how many people use rspec these days, what's the global cost of this context switching in hours of lost productivity?

What's wrong with have_size(n)? Does what it says on the tin.

gma commented Nov 10, 2011

I appreciate the effort to improve it after my slightly ranty gist, but I think the proposed change misses the mark slightly.

Good programmers are inquisitive. If they see something they don't understand the need for, they investigate. They're driven by a need to understand the code in front of them. If you show a good programmer some code with a seemingly unnecessary parameter or method call in it, they'll say "hang on, whats going on here?" and take a diversion from what they were meant to be doing in order to read the source. If that parameter/method serves no actual purpose you've just wasted the good programmer's time and energy.

New and inquisitive programmers who are destined to become good programmers are afflicted with the same curiousity.

Good APIs are self explanatory.

When you consider how many people use rspec these days, what's the global cost of this context switching in hours of lost productivity?

What's wrong with have_size(n)? Does what it says on the tin.

@gma

This comment has been minimized.

Show comment Hide comment
@gma

gma Nov 10, 2011

@rgarner – There's a cost associated with including multiple ways of doing anything in an API. There's the overhead of maintaining the extra code, and there's the cognitive load on all the people who use the library. You find your self wondering what the recommended way is, whether two things are really equivalent, and whether or not you should stop what you're doing to go and find a tutorial. API creep is rarely good news, but changing API without deprecating old API isn't necessarily the answer either.

gma commented Nov 10, 2011

@rgarner – There's a cost associated with including multiple ways of doing anything in an API. There's the overhead of maintaining the extra code, and there's the cognitive load on all the people who use the library. You find your self wondering what the recommended way is, whether two things are really equivalent, and whether or not you should stop what you're doing to go and find a tutorial. API creep is rarely good news, but changing API without deprecating old API isn't necessarily the answer either.

@rgarner

This comment has been minimized.

Show comment Hide comment
@rgarner

rgarner Nov 11, 2011

@gma one of the things I love about RSpec is the meaningful failure messages. What's wrong with have_size(n) is that you're giving less info to the matcher. If I see a CI build fail with a dozen messages suddenly (not uncommon) it's best if those messages give me as many clues as possible as to what exactly is wrong without me needing to do too much detective work. A failure message from customer.should have(2, :orders) might tell me Expected 2 orders, got 1, whereas have_size(n) would say something more with no object like Expected 2 items, got 1. Now you might reasonably say "But your test name should tell you this", but one of the things I also like about RSpec is that I'm not obliged to waste time coming up with test names when the spec is simple enough to explain itself. I can say it { should have(2, :orders) } (provided I've previously said that subject { some_customer }) and RSpec will tell me that "It should have 2 orders" failed.

I'm not sure about your "global cost of context switching" point with respect to RSpec. I think I paid some cost a long time ago (understanding what a matcher is, understanding what an example group does). I don't need to understand all of RSpec to be able to use it effectively. If I spent all my time diving into every parameter on every API I ever used I'd never ship anything :)

rgarner commented Nov 11, 2011

@gma one of the things I love about RSpec is the meaningful failure messages. What's wrong with have_size(n) is that you're giving less info to the matcher. If I see a CI build fail with a dozen messages suddenly (not uncommon) it's best if those messages give me as many clues as possible as to what exactly is wrong without me needing to do too much detective work. A failure message from customer.should have(2, :orders) might tell me Expected 2 orders, got 1, whereas have_size(n) would say something more with no object like Expected 2 items, got 1. Now you might reasonably say "But your test name should tell you this", but one of the things I also like about RSpec is that I'm not obliged to waste time coming up with test names when the spec is simple enough to explain itself. I can say it { should have(2, :orders) } (provided I've previously said that subject { some_customer }) and RSpec will tell me that "It should have 2 orders" failed.

I'm not sure about your "global cost of context switching" point with respect to RSpec. I think I paid some cost a long time ago (understanding what a matcher is, understanding what an example group does). I don't need to understand all of RSpec to be able to use it effectively. If I spent all my time diving into every parameter on every API I ever used I'd never ship anything :)

@rgarner

This comment has been minimized.

Show comment Hide comment
@rgarner

rgarner Nov 11, 2011

@gma I'm aware of the cost of maintaining old bits of API. My desire to keep the "old" way of doing it is/was an incorrect and selfish muscle memory-based one due to the number of times per day I use this style of matcher ;)

rgarner commented Nov 11, 2011

@gma I'm aware of the cost of maintaining old bits of API. My desire to keep the "old" way of doing it is/was an incorrect and selfish muscle memory-based one due to the number of times per day I use this style of matcher ;)

@dchelimsky

This comment has been minimized.

Show comment Hide comment
@dchelimsky

dchelimsky Nov 12, 2011

Owner

@rgarner - @gma summed up the reasons why we'll deprecate the old version and remove it later, should we make this change.

@gma - there are two different use cases for specifying size: when the subject is a collection, and when the subject owns a collection:

movies.size.should eq(3)
team.players.size.should eq(9)

have(n).xxx was created to express both cases with the same language:

movies.should have(3).potatoes # to quote your entertaining example
team.should have(9).players

I understand your rant, and agree that this creates a cognitive problem and I'm clearly open to addressing it, but I don't find that team.players.should have_size(9) offers any benefit over team.players.size.should eq(9), and it would be adding another matcher, further increasing cognitive load.

One alternative would be to deprecate the magic part entirely, but leave the delegation for the case when the subject owns the collection. This would leave us with:

movies.should have(3).items # deprecation warning in 2.x, error in 3.0
movies.size.should eq(3)    # would be the recommended alternative
team.should have(9).players # would remain supported for the "owned collection" case

In 3.0, the movies.should have(3).potatoes expression would raise an error unless movies had an items method. I think leaving support for team.should have(9).players would be reasonable for both newcomers (who should not have trouble understanding that players is a method on team) and previous users (who would face less change). The only people this would effect negatively (short term) would be those who are using the movies.should have(n).things form.

Thoughts?

Owner

dchelimsky commented Nov 12, 2011

@rgarner - @gma summed up the reasons why we'll deprecate the old version and remove it later, should we make this change.

@gma - there are two different use cases for specifying size: when the subject is a collection, and when the subject owns a collection:

movies.size.should eq(3)
team.players.size.should eq(9)

have(n).xxx was created to express both cases with the same language:

movies.should have(3).potatoes # to quote your entertaining example
team.should have(9).players

I understand your rant, and agree that this creates a cognitive problem and I'm clearly open to addressing it, but I don't find that team.players.should have_size(9) offers any benefit over team.players.size.should eq(9), and it would be adding another matcher, further increasing cognitive load.

One alternative would be to deprecate the magic part entirely, but leave the delegation for the case when the subject owns the collection. This would leave us with:

movies.should have(3).items # deprecation warning in 2.x, error in 3.0
movies.size.should eq(3)    # would be the recommended alternative
team.should have(9).players # would remain supported for the "owned collection" case

In 3.0, the movies.should have(3).potatoes expression would raise an error unless movies had an items method. I think leaving support for team.should have(9).players would be reasonable for both newcomers (who should not have trouble understanding that players is a method on team) and previous users (who would face less change). The only people this would effect negatively (short term) would be those who are using the movies.should have(n).things form.

Thoughts?

@gma

This comment has been minimized.

Show comment Hide comment
@gma

gma Jul 26, 2012

Personally, I'd deprecate everything other than your recommended alternative:

movies.size.should eq(3)

When I look at code like this…

team.should have(9).players

…I find myself uneasy about the idea of showing it to anybody. It's code that I can see causing "hey, how does that even run?" questions during code review with people who aren't familiar with it. That's not clear self explanatory code, which means it's not good code, which is why I'd deprecate it.

Incidentally, over the last six months I've gotten into a handful of discussions about the various flavours of Ruby testing frameworks (typically in the pub, after a conference or user group). I've spoken to quite a few people (some very experienced, some even professionally training people in RSpec and Cucumber) about the above, and have asked them all what my potatoes method does. I scribble it down for them on a scrap of paper, so there can be no confusion, and have then asked "what object does potatoes get called on?"

Nobody has yet already known the answer before I asked them, which suggests that this corner of the API isn't common knowledge. I think that strengthens the deprecation case.

Everybody has hazarded a guess. Some have told me that it obviously gets called on the subject (i.e. team has a players method), and have wondered why I'm wasting their time with such a trivial question. Some have stared at my code snippet and said they have absolutely no idea what it's doing.

All have been surprised when I tell them. Those more confident in their understanding of what was actually happening have looked rather taken aback. Embarrassed, even.

gma commented Jul 26, 2012

Personally, I'd deprecate everything other than your recommended alternative:

movies.size.should eq(3)

When I look at code like this…

team.should have(9).players

…I find myself uneasy about the idea of showing it to anybody. It's code that I can see causing "hey, how does that even run?" questions during code review with people who aren't familiar with it. That's not clear self explanatory code, which means it's not good code, which is why I'd deprecate it.

Incidentally, over the last six months I've gotten into a handful of discussions about the various flavours of Ruby testing frameworks (typically in the pub, after a conference or user group). I've spoken to quite a few people (some very experienced, some even professionally training people in RSpec and Cucumber) about the above, and have asked them all what my potatoes method does. I scribble it down for them on a scrap of paper, so there can be no confusion, and have then asked "what object does potatoes get called on?"

Nobody has yet already known the answer before I asked them, which suggests that this corner of the API isn't common knowledge. I think that strengthens the deprecation case.

Everybody has hazarded a guess. Some have told me that it obviously gets called on the subject (i.e. team has a players method), and have wondered why I'm wasting their time with such a trivial question. Some have stared at my code snippet and said they have absolutely no idea what it's doing.

All have been surprised when I tell them. Those more confident in their understanding of what was actually happening have looked rather taken aback. Embarrassed, even.

@dchelimsky

This comment has been minimized.

Show comment Hide comment
@dchelimsky

dchelimsky Jul 27, 2012

Owner

I'm open to deprecating that behavior, though I may prefer to deprecate it in the 3.0 rollout and make it an error in 4.0. This is behavior that has been supported for a long time and I'd like it to be deprecated for a long time before we remove it.

/cc @myronmarston @alindeman @spicycode @patmaddox

Owner

dchelimsky commented Jul 27, 2012

I'm open to deprecating that behavior, though I may prefer to deprecate it in the 3.0 rollout and make it an error in 4.0. This is behavior that has been supported for a long time and I'd like it to be deprecated for a long time before we remove it.

/cc @myronmarston @alindeman @spicycode @patmaddox

@myronmarston

This comment has been minimized.

Show comment Hide comment
@myronmarston

myronmarston Jul 27, 2012

Owner

I'm in favor of deprecating the have matchers as well. The method missing stuff has always struck me as confusing and unnecessary.

have(n, :things) is a definite improvement, but it's not clear what the :things parameter does, if anything. Readability is great, but as a programmer, you assume that every argument serves some functional purpose, so I think the :things in expect(array).to have(10, :things) is still confusing.

I've been using simple equality/comparison matchers on the size for a long time rather than the have matchers due to the confusion.

I'm open to deprecating that behavior, though I may prefer to deprecate it in the 3.0 rollout and make it an error in 4.0. This is behavior that has been supported for a long time and I'd like it to be deprecated for a long time before we remove it.

This sounds wise to me.

Owner

myronmarston commented Jul 27, 2012

I'm in favor of deprecating the have matchers as well. The method missing stuff has always struck me as confusing and unnecessary.

have(n, :things) is a definite improvement, but it's not clear what the :things parameter does, if anything. Readability is great, but as a programmer, you assume that every argument serves some functional purpose, so I think the :things in expect(array).to have(10, :things) is still confusing.

I've been using simple equality/comparison matchers on the size for a long time rather than the have matchers due to the confusion.

I'm open to deprecating that behavior, though I may prefer to deprecate it in the 3.0 rollout and make it an error in 4.0. This is behavior that has been supported for a long time and I'd like it to be deprecated for a long time before we remove it.

This sounds wise to me.

@patmaddox

This comment has been minimized.

Show comment Hide comment
@patmaddox

patmaddox Jul 27, 2012

Contributor

I love the have(11).players syntax. I'd be sad if it went away.

Contributor

patmaddox commented Jul 27, 2012

I love the have(11).players syntax. I'd be sad if it went away.

@myronmarston

This comment has been minimized.

Show comment Hide comment
@myronmarston

myronmarston Jul 27, 2012

Owner

@patmaddox -- what value do you find in it over players.size.should eq(11)?

Owner

myronmarston commented Jul 27, 2012

@patmaddox -- what value do you find in it over players.size.should eq(11)?

@patmaddox

This comment has been minimized.

Show comment Hide comment
@patmaddox

patmaddox Jul 27, 2012

Contributor

more readable. same reason I prefer players.size.should eq(1) over assert_equal 1, players.size

Contributor

patmaddox commented Jul 27, 2012

more readable. same reason I prefer players.size.should eq(1) over assert_equal 1, players.size

@gma

This comment has been minimized.

Show comment Hide comment
@gma

gma Jul 27, 2012

It's all so subjective though isn't it.

I've had people ask me "so what object does eq get called on?". Just one conversation like that and any speed improvement you've perceived over the last twelve months goes right out the window.

And to me, readable isn't relevant. It's comprehension that matters. I'd rather understand it than read it while assuming I've understood it.

gma commented Jul 27, 2012

It's all so subjective though isn't it.

I've had people ask me "so what object does eq get called on?". Just one conversation like that and any speed improvement you've perceived over the last twelve months goes right out the window.

And to me, readable isn't relevant. It's comprehension that matters. I'd rather understand it than read it while assuming I've understood it.

@vassilevsky

This comment has been minimized.

Show comment Hide comment
@vassilevsky

vassilevsky Feb 8, 2013

IMHO, the ability to write have(42).things is very important. Because Rails.

In Rails, you can write has_many :things in a model. The model then has the things method. It is there. You can call it. It becomes natural for a developer to use it. Writing model.should have(N).things in a test is then very natural. The developer does not care how that works. He or she added that method to the model and expects it to have it at all times.

Please do not deprecate it.

IMHO, the ability to write have(42).things is very important. Because Rails.

In Rails, you can write has_many :things in a model. The model then has the things method. It is there. You can call it. It becomes natural for a developer to use it. Writing model.should have(N).things in a test is then very natural. The developer does not care how that works. He or she added that method to the model and expects it to have it at all times.

Please do not deprecate it.

@myronmarston

This comment has been minimized.

Show comment Hide comment
@myronmarston

myronmarston Feb 9, 2013

Owner

@vassilevsky - there are pros and cons to this matcher. I'm leaning towards moving it into a separate gem -- then it's still available for those (like yourself) who want to opt-in to use it, but isn't shipped with rspec-expectations.

Owner

myronmarston commented Feb 9, 2013

@vassilevsky - there are pros and cons to this matcher. I'm leaning towards moving it into a separate gem -- then it's still available for those (like yourself) who want to opt-in to use it, but isn't shipped with rspec-expectations.

@soulcutter

This comment has been minimized.

Show comment Hide comment
@soulcutter

soulcutter Feb 21, 2013

Member

My two cents, I like the "owned collection" syntax, and agree with @dchelimsky 's comments from a year ago regarding that. Moving it into a separate gem is either a slow death for that syntax, or if it is popular then it fragments the syntax with which people be rspeccin'. Deprecate generic things, keep owned collections, and don't throw out a nice DSL.

I really think the confusion surrounds owned collections vs size, if the feature is only for owned collections it totally makes sense.

Member

soulcutter commented Feb 21, 2013

My two cents, I like the "owned collection" syntax, and agree with @dchelimsky 's comments from a year ago regarding that. Moving it into a separate gem is either a slow death for that syntax, or if it is popular then it fragments the syntax with which people be rspeccin'. Deprecate generic things, keep owned collections, and don't throw out a nice DSL.

I really think the confusion surrounds owned collections vs size, if the feature is only for owned collections it totally makes sense.

@samuil

This comment has been minimized.

Show comment Hide comment
@samuil

samuil Mar 11, 2013

Contributor

Hmm, It would be quite hard to migrate to, as it would break compatibility, but have(42, additional_arg) could actually perform collection_item === additional_arg when counting, and use return value to count this item or not. It would be meaningful - you can check some boolean method, class etc., and less mysterious for programmers.

expect(array).to have(42, Player) does not have superb natural-language-like readability because of wrong plurality, but can actually do something useful and should be easy to understand.

Contributor

samuil commented Mar 11, 2013

Hmm, It would be quite hard to migrate to, as it would break compatibility, but have(42, additional_arg) could actually perform collection_item === additional_arg when counting, and use return value to count this item or not. It would be meaningful - you can check some boolean method, class etc., and less mysterious for programmers.

expect(array).to have(42, Player) does not have superb natural-language-like readability because of wrong plurality, but can actually do something useful and should be easy to understand.

@samphippen

This comment has been minimized.

Show comment Hide comment
@samphippen

samphippen Jul 21, 2013

Owner

Now that this is being extracted into a gem, can we close this issue?

Owner

samphippen commented Jul 21, 2013

Now that this is being extracted into a gem, can we close this issue?

@dchelimsky

This comment has been minimized.

Show comment Hide comment
@dchelimsky

dchelimsky Jul 21, 2013

Owner

@samphippen I think we can close it because we've decided not to extend the have matcher in this way regardless of whether have moves to it's own gem. Seeing as I opened it, I'll close it as well :)

Owner

dchelimsky commented Jul 21, 2013

@samphippen I think we can close it because we've decided not to extend the have matcher in this way regardless of whether have moves to it's own gem. Seeing as I opened it, I'll close it as well :)

@dchelimsky dchelimsky closed this Jul 21, 2013

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