Proposal: raise error if more than one count key was passed #1260

Closed
abotalov opened this Issue Feb 24, 2014 · 9 comments

Projects

None yet

3 participants

@abotalov
Collaborator

#1232 added possibility to pass more than one count option (by count option I mean any of :count, :minimum, :maximum, :between).

But to support this ability properly we should:

  • raise errors if conflicting options were passed (e.g. count: 3, minimum: 4)
  • change failure message. Example keys: minimum: 2, maximum: 5, between: 3..6. Here are choices that I think we have:
    • normalize keys so error message will include from 3 to 5 times
    • just append keys so error message will include at least 2 times, at most 5 times and between 3 to 6 times

In any case it would require to write some code to support it properly. But I don't really see a use case when this ability may be useful. I propose to raise error if more than one count key has been passed.

cc @tmertens (as you added this feature)

Collaborator

Why would we want to ban the use of minimum and maximum together?

Collaborator

Because IMO it's easier to use :between instead

Collaborator

I think the intention of a test specifying minimum: 3, maximum: 6 is clearer than between: 3..6 or between 3...7 - but maybe I'm just biased with regard to the use of ranges because I've seen too many people screw up the difference between inclusive and exclusive ranges

Collaborator

@twalpole Do you think that issue that you talk about is worth adding code for stuff outlined in issue description?

Collaborator

Actually I don't think its worth worrying about conflicting options at all. If a user adds conflicting options the test will fail and they will then have to look at why the test is failing. As for the error message, appending them together as in your second example would be my preference since the error message then relates directly to the count options used

Collaborator
jnicklas commented Mar 2, 2014

I agree with @twalpole on this. No need to go too crazy on this. If a user wants to choose options which might conflict then that's their prerogrative.

@jnicklas jnicklas closed this Mar 2, 2014
@abotalov abotalov self-assigned this Mar 2, 2014
@abotalov abotalov reopened this Mar 2, 2014
@abotalov abotalov closed this Mar 2, 2014
@abotalov abotalov removed their assignment Mar 2, 2014
Collaborator
abotalov commented Jul 8, 2014

If a user adds conflicting options the test will fail and they will then have to look at why the test is failing.

It doesn't seem to be true to me as Helpers#matches_count? returns just after checking count option. It won't even check others. So the options may be conflicting but the tests will pass.

@abotalov abotalov reopened this Jul 8, 2014
Collaborator
twalpole commented Jul 8, 2014

Please open a new issue about it not checking all the options if count is specified, we will not be raising an error if more than one count key is passed though

@twalpole twalpole closed this Jul 8, 2014
@twalpole twalpole locked and limited conversation to collaborators Jul 8, 2014
@twalpole twalpole unlocked this conversation Jul 8, 2014
Collaborator
twalpole commented Jul 8, 2014

Also note that the behavior you're complaining about is behavior that is actually tested for in all_spec.rb, and documented in Capybara::Helpers::matches_count, which is referred to from the finders #all documentation - so it definitely won't be changed before 3.0 at the earliest

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