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

Change matcher protocol and definition API to not reference `should` and `should_not` #270

Closed
myronmarston opened this Issue Jun 11, 2013 · 21 comments

Comments

Projects
None yet
10 participants
@myronmarston
Member

myronmarston commented Jun 11, 2013

We've been moving in the direction of expect from should but there are "legacy" bits of the API that still speak in terms of should and should_not. In 3.0 I'd like to come up with an improved API that doesn't use these terms while still maintaining compatibility for code out there that is based on those APIs, and then we can remove the old APIs in 4.0.

The specific APIs:

  • failure_message_for_should and failure_message_for_should_not as part of the matcher protocol and custom matcher DSL.
  • match_for_should and match_for_should_not as part of the custom matcher DSL.
  • RSpec::Matchers.last_should.

I don't have a proposal for new names for these yet...this is mostly a place holder issue to facilitate discussion :).

@samphippen

This comment has been minimized.

Show comment
Hide comment
@samphippen

samphippen Jun 21, 2013

Member

So I'm thinking s/should/expectation/ on most of these and s/should not/negative expectation. That is:

failure_message_for_expectation
failure_message_for_negative_expectation
match_for_expectation
match_for_negative_expectation
RSpec::Matches.last_expectation

Thoughts?

Member

samphippen commented Jun 21, 2013

So I'm thinking s/should/expectation/ on most of these and s/should not/negative expectation. That is:

failure_message_for_expectation
failure_message_for_negative_expectation
match_for_expectation
match_for_negative_expectation
RSpec::Matches.last_expectation

Thoughts?

@zaphod42

This comment has been minimized.

Show comment
Hide comment
@zaphod42

zaphod42 Jul 17, 2013

The terminology on matchers can use the domain of matching and mismatching:

  • failure_message_for_match and failure_message_for_mismatch
  • matches_when and mismatches_when

zaphod42 commented Jul 17, 2013

The terminology on matchers can use the domain of matching and mismatching:

  • failure_message_for_match and failure_message_for_mismatch
  • matches_when and mismatches_when
@booch

This comment has been minimized.

Show comment
Hide comment
@booch

booch Jul 17, 2013

I'm not sure these are better, but:

failure_message
negative_failure_message
match
negative_match

booch commented Jul 17, 2013

I'm not sure these are better, but:

failure_message
negative_failure_message
match
negative_match
@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Jul 18, 2013

Member

I like the shorter ones that @booch suggests, although haven't we been discussing using === for match?

Member

JonRowe commented Jul 18, 2013

I like the shorter ones that @booch suggests, although haven't we been discussing using === for match?

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Jul 18, 2013

Member

I like @samphippen's suggestions, personally.

The terminology on matchers can use the domain of matching and mismatching:

Interesting idea, but IMO, "mismatch" gives the wrong sense: to me, a mismatch, suggests a failure of an attempted positive expectation. It doesn't suggest a negative expectation to me.

haven't we been discussing using === for match?

On the built-in matchers, we've discussed aliasing match? to === rather than == (as it is aliased to now) to better align with the semantics of these operators, but we're not considering making === a required part of the matcher protocol rather than match?.

Member

myronmarston commented Jul 18, 2013

I like @samphippen's suggestions, personally.

The terminology on matchers can use the domain of matching and mismatching:

Interesting idea, but IMO, "mismatch" gives the wrong sense: to me, a mismatch, suggests a failure of an attempted positive expectation. It doesn't suggest a negative expectation to me.

haven't we been discussing using === for match?

On the built-in matchers, we've discussed aliasing match? to === rather than == (as it is aliased to now) to better align with the semantics of these operators, but we're not considering making === a required part of the matcher protocol rather than match?.

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Jul 18, 2013

Member

Ah ok, brain misfire :)

Member

JonRowe commented Jul 18, 2013

Ah ok, brain misfire :)

@jnicklas

This comment has been minimized.

Show comment
Hide comment
@jnicklas

jnicklas Jul 24, 2013

+1 on @booch's suggestion.

jnicklas commented Jul 24, 2013

+1 on @booch's suggestion.

@cupakromer

This comment has been minimized.

Show comment
Hide comment
@cupakromer

cupakromer Jul 29, 2013

Member

I've gone back and forth on which I like better. I think my sticking point with @booch's suggestion is the negative_failure_message and negative_failure_match makes my brain stumble. It reads like a double negative. So I'm leaning more to @samphippen versions even those they are more verbose.

Member

cupakromer commented Jul 29, 2013

I've gone back and forth on which I like better. I think my sticking point with @booch's suggestion is the negative_failure_message and negative_failure_match makes my brain stumble. It reads like a double negative. So I'm leaning more to @samphippen versions even those they are more verbose.

@jnicklas

This comment has been minimized.

Show comment
Hide comment
@jnicklas

jnicklas Aug 7, 2013

How about negated_failure_message and negated_match as opposed to negative_failure_message and negative_match.

jnicklas commented Aug 7, 2013

How about negated_failure_message and negated_match as opposed to negative_failure_message and negative_match.

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Aug 7, 2013

Member

I like those, but what about flipping them? In fact... what about:

match
match_when_negated
failure_message
failure_message_when_negated
Member

JonRowe commented Aug 7, 2013

I like those, but what about flipping them? In fact... what about:

match
match_when_negated
failure_message
failure_message_when_negated
@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Aug 7, 2013

Member

I like negated over negative, I think. That said, there's something nice about the symmetry of:

positive_match
negative_match
positive_failure_message
negative_failure_message

...which doesn't line up with negated well.

Member

myronmarston commented Aug 7, 2013

I like negated over negative, I think. That said, there's something nice about the symmetry of:

positive_match
negative_match
positive_failure_message
negative_failure_message

...which doesn't line up with negated well.

@xaviershay

This comment has been minimized.

Show comment
Hide comment
@xaviershay

xaviershay Nov 25, 2013

Member

+1 these:

match
match_when_negated
failure_message
failure_message_when_negated

Clarity beats symmetry, and these ones have the benefit of mostly being terse.

Member

xaviershay commented Nov 25, 2013

+1 these:

match
match_when_negated
failure_message
failure_message_when_negated

Clarity beats symmetry, and these ones have the benefit of mostly being terse.

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Nov 25, 2013

Member

I don't like any of the options presented here, but I don't have a better suggestion. The words positive or negative or negated apply to the expectation, not the message. I think these names tell the right story:

failure_message_for_positive_expectation
failure_message_for_negative_expectation
match_for_positive_expectation
match_for_negative_expectation

... but obviously they are painfully long.

FYI - the original names were failure_message and negative_failure_message. I changed them to failure_message_for_should[_not] because negative_failure_message sounded like the word negative was describing the failure message rather than the expectation that failed.

I can almost get behind @myronmarston's suggestion ([positive|negative]_[match|failure_message]) because of the symmetry which, btw, I think lends clarity (even if clarity trumps).

Wish I had a better answer. Good luck!

Member

dchelimsky commented Nov 25, 2013

I don't like any of the options presented here, but I don't have a better suggestion. The words positive or negative or negated apply to the expectation, not the message. I think these names tell the right story:

failure_message_for_positive_expectation
failure_message_for_negative_expectation
match_for_positive_expectation
match_for_negative_expectation

... but obviously they are painfully long.

FYI - the original names were failure_message and negative_failure_message. I changed them to failure_message_for_should[_not] because negative_failure_message sounded like the word negative was describing the failure message rather than the expectation that failed.

I can almost get behind @myronmarston's suggestion ([positive|negative]_[match|failure_message]) because of the symmetry which, btw, I think lends clarity (even if clarity trumps).

Wish I had a better answer. Good luck!

@soulcutter

This comment has been minimized.

Show comment
Hide comment
@soulcutter

soulcutter Nov 25, 2013

Member

Of the proposals thus far, I prefer @myronmarston 's

positive_match
negative_match
positive_failure_message
negative_failure_message

The similarly-prefixed methods go together, which makes it clearer to me. Negated is descriptive, but will not be as self-evident.

Member

soulcutter commented Nov 25, 2013

Of the proposals thus far, I prefer @myronmarston 's

positive_match
negative_match
positive_failure_message
negative_failure_message

The similarly-prefixed methods go together, which makes it clearer to me. Negated is descriptive, but will not be as self-evident.

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Nov 25, 2013

Member

I still prefer:

match
match_when_negated
failure_message
failure_message_when_negated

As this way the method names are paired with their opposite, I find the prefixes not matching for the same functionality confusing.

Member

JonRowe commented Nov 25, 2013

I still prefer:

match
match_when_negated
failure_message
failure_message_when_negated

As this way the method names are paired with their opposite, I find the prefixes not matching for the same functionality confusing.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Nov 25, 2013

Member

Funny how it's hard to get consensus on this!

A few points of clarification (although these things may already be obvious):

  • For the current custom matcher DSL (that is, the methods available for users to call to define a matcher using RSpec::Matchers.define), the match methods are match, match_for_should and match_for_should_not. match_for_should is an alias of match that is there simply for symmetry if a user needs to define different logic for a negative expectation. I intend to keep match as the primary positive match API; depending on the negative name we choose we may have a symmetric alias of match but it's not something we necessarily have to do if there's no good symmetric alias.
  • The "match" methods of the matcher protocol are matches? and does_not_match? and I intend to keep those.
  • The pair of failure message methods will be used by both the matcher protocol and the custom matcher DSL. Currently in a define block you can define the positive failure message using failure_message_for_should { }, which defines a failure_message_for_should instance method. I imagine we'll keep the same kind of setup.

At this point, I think I like @JonRowe's suggestion the best:

match
match_when_negated
failure_message
failure_message_when_negated

In particular, failure_message_when_negated makes it clear that it's not the failure message itself that is negated but the match as a whole (in contrast to negative_failure_message which is a bit unclear as others have said).

Barring a better suggestion between now and however long it takes me to put up a PR for this, that's what I'll go with. Any objections?

Member

myronmarston commented Nov 25, 2013

Funny how it's hard to get consensus on this!

A few points of clarification (although these things may already be obvious):

  • For the current custom matcher DSL (that is, the methods available for users to call to define a matcher using RSpec::Matchers.define), the match methods are match, match_for_should and match_for_should_not. match_for_should is an alias of match that is there simply for symmetry if a user needs to define different logic for a negative expectation. I intend to keep match as the primary positive match API; depending on the negative name we choose we may have a symmetric alias of match but it's not something we necessarily have to do if there's no good symmetric alias.
  • The "match" methods of the matcher protocol are matches? and does_not_match? and I intend to keep those.
  • The pair of failure message methods will be used by both the matcher protocol and the custom matcher DSL. Currently in a define block you can define the positive failure message using failure_message_for_should { }, which defines a failure_message_for_should instance method. I imagine we'll keep the same kind of setup.

At this point, I think I like @JonRowe's suggestion the best:

match
match_when_negated
failure_message
failure_message_when_negated

In particular, failure_message_when_negated makes it clear that it's not the failure message itself that is negated but the match as a whole (in contrast to negative_failure_message which is a bit unclear as others have said).

Barring a better suggestion between now and however long it takes me to put up a PR for this, that's what I'll go with. Any objections?

@xaviershay

This comment has been minimized.

Show comment
Hide comment
@xaviershay

xaviershay Nov 25, 2013

Member

go man go

Member

xaviershay commented Nov 25, 2013

go man go

@soulcutter

This comment has been minimized.

Show comment
Hide comment
@soulcutter

soulcutter Nov 25, 2013

Member

No objections here.

Member

soulcutter commented Nov 25, 2013

No objections here.

@jnicklas

This comment has been minimized.

Show comment
Hide comment
@jnicklas

jnicklas Nov 25, 2013

I like those too.

jnicklas commented Nov 25, 2013

I like those too.

@samphippen

This comment has been minimized.

Show comment
Hide comment
@samphippen

samphippen Nov 25, 2013

Member

👍

Member

samphippen commented Nov 25, 2013

👍

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Dec 3, 2013

Member

Fixed by #373.

Member

myronmarston commented Dec 3, 2013

Fixed by #373.

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