When generating a failure message for expected args, don't use description if nil or empty #417

Merged
merged 1 commit into from Oct 2, 2013

Conversation

Projects
None yet
5 participants
@nddeluca
Contributor

nddeluca commented Sep 12, 2013

Currently, when setting a expectation with an argument with a blank description, the failure message lists no argument. This can be confusing, and should default to calling arg.inspect if the description is blank.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 12, 2013

Coverage Status

Coverage decreased (-0.08%) when pulling 203f662 on nddeluca:check-for-valid-arg-description into fd78578 on rspec:master.

Coverage Status

Coverage decreased (-0.08%) when pulling 203f662 on nddeluca:check-for-valid-arg-description into fd78578 on rspec:master.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Sep 12, 2013

Member

Currently, when setting a expectation with an argument with a blank description, the failure message lists no argument. This can be confusing, and should default to calling arg.inspect if the description is blank.

It seems bad for an object to implement description and then not to return a description. Can you just properly implement description on your object rather than adding this conditional checks to rspec-mocks? Can you show an example of where this is tripping you up?

Member

myronmarston commented Sep 12, 2013

Currently, when setting a expectation with an argument with a blank description, the failure message lists no argument. This can be confusing, and should default to calling arg.inspect if the description is blank.

It seems bad for an object to implement description and then not to return a description. Can you just properly implement description on your object rather than adding this conditional checks to rspec-mocks? Can you show an example of where this is tripping you up?

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Sep 12, 2013

Member

👎 Given this add's essentially two more conditional checks to each and every spec.

Member

JonRowe commented Sep 12, 2013

👎 Given this add's essentially two more conditional checks to each and every spec.

@nddeluca

This comment has been minimized.

Show comment
Hide comment
@nddeluca

nddeluca Sep 12, 2013

Contributor

@myronmarston I can understand that, but just because an object implements description doesn't mean that it must have a return value on instantiation. This tripped me when using an object that implemented description in it's interface, but the method(s) I was passing the object to did not use the description. I ended up setting the description only to obtain a helpful failure message.

@JonRowe Maybe I didn't read the source code carefully enough, but wouldn't these checks only occur for failing specs?

Contributor

nddeluca commented Sep 12, 2013

@myronmarston I can understand that, but just because an object implements description doesn't mean that it must have a return value on instantiation. This tripped me when using an object that implemented description in it's interface, but the method(s) I was passing the object to did not use the description. I ended up setting the description only to obtain a helpful failure message.

@JonRowe Maybe I didn't read the source code carefully enough, but wouldn't these checks only occur for failing specs?

@xaviershay

This comment has been minimized.

Show comment
Hide comment
@xaviershay

xaviershay Oct 1, 2013

Member

@myronmarston @JonRowe can you respond and enable us to push this to resolution?

Member

xaviershay commented Oct 1, 2013

@myronmarston @JonRowe can you respond and enable us to push this to resolution?

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Oct 1, 2013

Member

Mine and @myronmarston's original concern about extra conditionals still stand.

Member

JonRowe commented Oct 1, 2013

Mine and @myronmarston's original concern about extra conditionals still stand.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Oct 2, 2013

Member

You know, after thinking about this some more, I think this is a good change. While I think it would be poor practice for an object that is designed to be an rspec-mocks argument matcher to implement description but return nil or '', description is a common method name that RSpec doesn't have a monopoly on. Users can have their own objects (which may have requirements to return nil or '' from description in certain circumstances) and they may want to assert that a message is received with their object as an argument.

I'm going to merge this. Thanks, @nddeluca, and sorry about the delay!

Member

myronmarston commented Oct 2, 2013

You know, after thinking about this some more, I think this is a good change. While I think it would be poor practice for an object that is designed to be an rspec-mocks argument matcher to implement description but return nil or '', description is a common method name that RSpec doesn't have a monopoly on. Users can have their own objects (which may have requirements to return nil or '' from description in certain circumstances) and they may want to assert that a message is received with their object as an argument.

I'm going to merge this. Thanks, @nddeluca, and sorry about the delay!

myronmarston added a commit that referenced this pull request Oct 2, 2013

Merge pull request #417 from nddeluca/check-for-valid-arg-description
When generating a failure message for expected args, don't use description if nil or empty

@myronmarston myronmarston merged commit 3ff4185 into rspec:master Oct 2, 2013

1 check passed

default The Travis CI build passed
Details

myronmarston added a commit that referenced this pull request Oct 2, 2013

@nddeluca

This comment has been minimized.

Show comment
Hide comment
@nddeluca

nddeluca Oct 4, 2013

Contributor

Cool :) And no worries on the delay, glad to help!

Contributor

nddeluca commented Oct 4, 2013

Cool :) And no worries on the delay, glad to help!

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