Fixup change matcher #376

Merged
merged 8 commits into from Dec 4, 2013

2 participants

@myronmarston
RSpec member

No description provided.

myronmarston added some commits Dec 2, 2013
@myronmarston myronmarston Reword some failure messages to not reference should/should_not. 907dfc3
@myronmarston myronmarston Fix odd method formatting. 37a8a0e
@myronmarston myronmarston Reword an error message. 91c7863
@myronmarston myronmarston Doc the change matcher class. 198504f
@myronmarston myronmarston Refactor change matcher.
* Use separate classes for relative vs specific changes.
* This greatly simplifies the `matches?` and `failure_message`
  logic.
* Reduces the number of instance variables are floating around.
490648f
@soulcutter soulcutter commented on an outdated diff Dec 4, 2013
Changelog.md
@@ -13,6 +13,7 @@ Enhancements:
* `failure_message_for_should_not` => `failure_message_when_negated`
* `match_for_should` => `match`
* `match_for_should_not` => `match_when_negated`
+* Improve generated descriptions from `change` mather. (Myron Marston)
@soulcutter
RSpec member

This typo stands out a bit.

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

I like the refactor - it's much cleaner this way.

@soulcutter soulcutter commented on the diff Dec 4, 2013
lib/rspec/matchers/built_in/change.rb
end
- def to(to)
- @eval_after = true
- @expected_after = to
- self
+ def failure_message
+ if !matches_before?
@soulcutter
RSpec member

Seems like the order of this if/else could be reversed to save a negation, though I hesitate to even mention it because it's so minor of a nitpick

@myronmarston
RSpec member

This is the failure message, so we're figuring out which part failed, and then returning a corresponding failure message. If we switch it, we'll be figuring out which part didn't fail, and then returning a failure message for the part that did (that wasn't in the conditional) so it would be much more confusing, IMO.

@soulcutter
RSpec member

I can see it both ways. I'm fine with leaving it as-is, it's plenty readable.

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

@soulcutter -- I pushed a couple more commits, which fix #154.

@soulcutter soulcutter merged commit d1f71c7 into master Dec 4, 2013

1 check passed

Details default The Travis CI build passed
@soulcutter soulcutter deleted the fixup-change-matcher branch Dec 4, 2013
@soulcutter
RSpec member

Looks good - definitely appreciate the attention to documentation also.

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