Skip to content

Commit

Permalink
indicative docstrings
Browse files Browse the repository at this point in the history
This was a pull request (#71) from Tom Stuart that I didn't merge until several
months later, so I had to address some merge conflicts. In addition, the
original intent was to offer the end user a configuration option for the
"voice" of the message (e.g. "should be x" vs "is x"), but I made it check if
the matcher responds to new methods docstring_for_should or
docstring_for_should_not and use those. This way all the internal matchers do
the right thing, but external matchers will still work without rspec trying to
convert the mode of it's docstring.
  • Loading branch information
Tom Stuart authored and dchelimsky committed Sep 14, 2012
1 parent d3d8672 commit 2f0d105
Show file tree
Hide file tree
Showing 30 changed files with 341 additions and 169 deletions.
24 changes: 12 additions & 12 deletions features/custom_matchers/define_matcher.feature
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ Feature: define matcher
When I run `rspec ./matcher_with_default_message_spec.rb --format documentation` When I run `rspec ./matcher_with_default_message_spec.rb --format documentation`
Then the exit status should not be 0 Then the exit status should not be 0


And the output should contain "should be a multiple of 3" And the output should contain "is a multiple of 3"
And the output should contain "should not be a multiple of 4" And the output should contain "is not a multiple of 4"
And the output should contain "Failure/Error: it {should be_a_multiple_of(4)}" And the output should contain "Failure/Error: it {should be_a_multiple_of(4)}"
And the output should contain "Failure/Error: it {should_not be_a_multiple_of(3)}" And the output should contain "Failure/Error: it {should_not be_a_multiple_of(3)}"


Expand Down Expand Up @@ -94,17 +94,19 @@ Feature: define matcher
And the stdout should contain "1 example, 1 failure" And the stdout should contain "1 example, 1 failure"
And the stdout should contain "expected that 9 would not be a multiple of 3" And the stdout should contain "expected that 9 would not be a multiple of 3"


Scenario: overriding the description Scenario: overriding the docstrings
Given a file named "matcher_overriding_description_spec.rb" with: Given a file named "matcher_overriding_description_spec.rb" with:
""" """
require 'rspec/expectations' require 'rspec/expectations'
RSpec::Matchers.define :be_a_multiple_of do |expected| RSpec::Matchers.define :be_a_multiple_of do |expected|
match do |actual| match do |actual|
actual % expected == 0 actual % expected == 0
end end
description do docstring_for_should do
"be multiple of #{expected}" "is multiple of #{expected}"
end
docstring_for_should_not do
"is not multiple of #{expected}"
end end

This comment has been minimized.

Copy link
@myronmarston

myronmarston Sep 14, 2012

Member

Given that we are moving away from the should/should_not syntax I think it would make sense to choose different names for these if we can come up with something that makes sense. How about:

positive_docstring do
end

negative_docstring do
end

This comment has been minimized.

Copy link
@dchelimsky

dchelimsky Sep 14, 2012

Contributor

These names align w/ the existing failure_message_for_should and failure_message_for_should_not. These used to be positive_failure_message and negative_failure_message.

Whatever naming scheme we agree on, it should be consistent across docstrings and failure messages and we need to continue to support the old names for a long, long time.

This comment has been minimized.

Copy link
@myronmarston

myronmarston Sep 26, 2012

Member

@alindeman -- any thoughts on this? I agree with @dchelimsky that we need to be consistent here and be concerned with forwards/backwards compatibility...but it also seems very odd to add new methods that are named after deprecated methods.

This comment has been minimized.

Copy link
@dchelimsky

dchelimsky Sep 27, 2012

Contributor

When I started to implement this (before you reminded me that @mortice already had), I went with descriptions[:positive] and descriptions[:negative] - we could use that and then add a similar method for the failure messages. I don't think that we can really deprecate the old methods for a long, long time though.

This comment has been minimized.

Copy link
@myronmarston

myronmarston Sep 27, 2012

Member

Ironically, @mortice's pull request originally started with that API as well--a method that returned a hash with :positive and :negative entries in it.

At the time, I felt like this was a bit of an awkward API: I can't think of any other API in any standard ruby libraries that return a hash of two elements with 2 keys that are always in the hash. Separate methods still makes more sense to me.

That said, if you're fine with positive and negative keys in the hash, why are you not fine with positive and negative in the method names?

This comment has been minimized.

Copy link
@dchelimsky

dchelimsky Sep 27, 2012

Contributor

That said, if you're fine with positive and negative keys in the hash, why are you not fine with positive and negative in the method names?

It's not bad for the docstrings, but we want to land on a naming scheme that works equally well for docstrings and failure messages and I find negative_failure_message less confusing than failure_message[:negative] - the latter of which separates the polarity from the method name.

I'm not excited about either option at the moment, so more ideas welcome.

end end
Expand All @@ -119,14 +121,13 @@ Feature: define matcher
When I run `rspec ./matcher_overriding_description_spec.rb --format documentation` When I run `rspec ./matcher_overriding_description_spec.rb --format documentation`
Then the exit status should be 0 Then the exit status should be 0
And the stdout should contain "2 examples, 0 failures" And the stdout should contain "2 examples, 0 failures"
And the stdout should contain "should be multiple of 3" And the stdout should contain "is multiple of 3"
And the stdout should contain "should not be multiple of 4" And the stdout should contain "is not multiple of 4"


Scenario: with no args Scenario: with no args
Given a file named "matcher_with_no_args_spec.rb" with: Given a file named "matcher_with_no_args_spec.rb" with:
""" """
require 'rspec/expectations' require 'rspec/expectations'
RSpec::Matchers.define :have_7_fingers do RSpec::Matchers.define :have_7_fingers do
match do |thing| match do |thing|
thing.fingers.length == 7 thing.fingers.length == 7
Expand All @@ -144,13 +145,12 @@ Feature: define matcher
When I run `rspec ./matcher_with_no_args_spec.rb --format documentation` When I run `rspec ./matcher_with_no_args_spec.rb --format documentation`
Then the exit status should be 0 Then the exit status should be 0
And the stdout should contain "1 example, 0 failures" And the stdout should contain "1 example, 0 failures"
And the stdout should contain "should have 7 fingers" And the stdout should contain "has 7 fingers"


Scenario: with multiple args Scenario: with multiple args
Given a file named "matcher_with_multiple_args_spec.rb" with: Given a file named "matcher_with_multiple_args_spec.rb" with:
""" """
require 'rspec/expectations' require 'rspec/expectations'
RSpec::Matchers.define :be_the_sum_of do |a,b,c,d| RSpec::Matchers.define :be_the_sum_of do |a,b,c,d|
match do |sum| match do |sum|
a + b + c + d == sum a + b + c + d == sum
Expand All @@ -164,7 +164,7 @@ Feature: define matcher
When I run `rspec ./matcher_with_multiple_args_spec.rb --format documentation` When I run `rspec ./matcher_with_multiple_args_spec.rb --format documentation`
Then the exit status should be 0 Then the exit status should be 0
And the stdout should contain "1 example, 0 failures" And the stdout should contain "1 example, 0 failures"
And the stdout should contain "should be the sum of 1, 2, 3, and 4" And the stdout should contain "is the sum of 1, 2, 3, and 4"


Scenario: with helper methods Scenario: with helper methods
Given a file named "matcher_with_internal_helper_spec.rb" with: Given a file named "matcher_with_internal_helper_spec.rb" with:
Expand Down
Original file line number Original file line Diff line number Diff line change
@@ -1,7 +1,7 @@
Feature: define matcher with fluent interface Feature: define matcher with fluent interface


Use the chain() method to define matchers with a fluent interface. Use the chain() method to define matchers with a fluent interface.

Scenario: chained method with argumetn Scenario: chained method with argumetn
Given a file named "between_spec.rb" with: Given a file named "between_spec.rb" with:
""" """
Expand All @@ -21,4 +21,4 @@ Feature: define matcher with fluent interface
""" """
When I run `rspec between_spec.rb --format documentation` When I run `rspec between_spec.rb --format documentation`
Then the output should contain "1 example, 0 failures" Then the output should contain "1 example, 0 failures"
And the output should contain "should be bigger than 4" And the output should contain "is bigger than 4"
14 changes: 7 additions & 7 deletions features/implicit_docstrings.feature
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@ Feature: implicit docstrings


When I run `rspec ./implicit_docstrings_spec.rb -fdoc` When I run `rspec ./implicit_docstrings_spec.rb -fdoc`


Then the output should contain "should be < 5" Then the output should contain "is < 5"
And the output should contain "should include 2" And the output should contain "includes 2"
And the output should contain "should respond to #size" And the output should contain "responds to #size"


Scenario: run failing examples Scenario: run failing examples
Given a file named "failing_implicit_docstrings_spec.rb" with: Given a file named "failing_implicit_docstrings_spec.rb" with:
Expand All @@ -46,7 +46,7 @@ Feature: implicit docstrings


When I run `rspec ./failing_implicit_docstrings_spec.rb -fdoc` When I run `rspec ./failing_implicit_docstrings_spec.rb -fdoc`


Then the output should contain "should equal 2" Then the output should contain "equals 2"
And the output should contain "should be > 5" And the output should contain "is > 5"
And the output should contain "should include 4" And the output should contain "includes 4"
And the output should contain "should not respond to #size" And the output should contain "does not respond to #size"
3 changes: 2 additions & 1 deletion lib/rspec/expectations.rb
Original file line number Original file line Diff line number Diff line change
@@ -1,3 +1,4 @@
require 'rspec/core'
require 'rspec/expectations/extensions' require 'rspec/expectations/extensions'
require 'rspec/matchers' require 'rspec/matchers'
require 'rspec/expectations/expectation_target' require 'rspec/expectations/expectation_target'
Expand Down Expand Up @@ -37,7 +38,7 @@ module RSpec
# `eq.failure_message_for_should_not`. # `eq.failure_message_for_should_not`.
# #
# rspec-expectations ships with a standard set of useful matchers, and writing # rspec-expectations ships with a standard set of useful matchers, and writing
# your own matchers is quite simple. # your own matchers is quite simple.
# #
# See [RSpec::Matchers](../RSpec/Matchers) for more information about the # See [RSpec::Matchers](../RSpec/Matchers) for more information about the
# built-in matchers that ship with rspec-expectations, and how to write your # built-in matchers that ship with rspec-expectations, and how to write your
Expand Down
22 changes: 13 additions & 9 deletions lib/rspec/matchers.rb
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -93,7 +93,11 @@ module RSpec
# # generate and return the appropriate string. # # generate and return the appropriate string.
# end # end
# #
# description do # docstring_for_should do
# # generate and return the appropriate string.
# end
#
# docstring_for_should_not do
# # generate and return the appropriate string. # # generate and return the appropriate string.
# end # end
# end # end
Expand Down Expand Up @@ -214,7 +218,7 @@ def be_nil
# #
# Given true, false, or nil, will pass if actual value is true, false or # Given true, false, or nil, will pass if actual value is true, false or
# nil (respectively). Given no args means the caller should satisfy an if # nil (respectively). Given no args means the caller should satisfy an if
# condition (to be or not to be). # condition (to be or not to be).
# #
# Predicates are any Ruby method that ends in a "?" and returns true or # Predicates are any Ruby method that ends in a "?" and returns true or
# false. Given be_ followed by arbitrary_predicate (without the "?"), # false. Given be_ followed by arbitrary_predicate (without the "?"),
Expand Down Expand Up @@ -246,7 +250,7 @@ def be_a(klass)
def be_an_instance_of(expected) def be_an_instance_of(expected)
BuiltIn::BeAnInstanceOf.new(expected) BuiltIn::BeAnInstanceOf.new(expected)
end end

alias_method :be_instance_of, :be_an_instance_of alias_method :be_instance_of, :be_an_instance_of


# Passes if actual.kind_of?(expected) # Passes if actual.kind_of?(expected)
Expand Down Expand Up @@ -288,20 +292,20 @@ def be_within(delta)
# @example # @example
# #
# lambda { # lambda {
# team.add_player(player) # team.add_player(player)
# }.should change(roster, :count) # }.should change(roster, :count)
# #
# lambda { # lambda {
# team.add_player(player) # team.add_player(player)
# }.should change(roster, :count).by(1) # }.should change(roster, :count).by(1)
# #
# lambda { # lambda {
# team.add_player(player) # team.add_player(player)
# }.should change(roster, :count).by_at_least(1) # }.should change(roster, :count).by_at_least(1)
# #
# lambda { # lambda {
# team.add_player(player) # team.add_player(player)
# }.should change(roster, :count).by_at_most(1) # }.should change(roster, :count).by_at_most(1)
# #
# string = "string" # string = "string"
# lambda { # lambda {
Expand All @@ -311,7 +315,7 @@ def be_within(delta)
# lambda { # lambda {
# person.happy_birthday # person.happy_birthday
# }.should change(person, :birthday).from(32).to(33) # }.should change(person, :birthday).from(32).to(33)
# #
# lambda { # lambda {
# employee.develop_great_new_social_networking_app # employee.develop_great_new_social_networking_app
# }.should change(employee, :title).from("Mail Clerk").to("CEO") # }.should change(employee, :title).from("Mail Clerk").to("CEO")
Expand Down Expand Up @@ -523,7 +527,7 @@ def raise_error(error=Exception, message=nil, &block)
# provided. Names can be Strings or Symbols. # provided. Names can be Strings or Symbols.
# #
# @example # @example
# #
def respond_to(*names) def respond_to(*names)
BuiltIn::RespondTo.new(*names) BuiltIn::RespondTo.new(*names)
end end
Expand Down
48 changes: 44 additions & 4 deletions lib/rspec/matchers/built_in/be.rb
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -15,6 +15,14 @@ def failure_message_for_should
def failure_message_for_should_not def failure_message_for_should_not
"expected: non-true value\n got: #{actual.inspect}" "expected: non-true value\n got: #{actual.inspect}"
end end

def docstring_for_should
"is true"
end

def docstring_for_should_not
"is not true"
end
end end


class BeFalse < BaseMatcher class BeFalse < BaseMatcher
Expand All @@ -29,6 +37,14 @@ def failure_message_for_should
def failure_message_for_should_not def failure_message_for_should_not
"expected: non-false value\n got: #{actual.inspect}" "expected: non-false value\n got: #{actual.inspect}"
end end

def docstring_for_should
"is false"
end

def docstring_for_should_not
"is not false"
end
end end


class BeNil < BaseMatcher class BeNil < BaseMatcher
Expand All @@ -43,6 +59,14 @@ def failure_message_for_should
def failure_message_for_should_not def failure_message_for_should_not
"expected: not nil\n got: nil" "expected: not nil\n got: nil"
end end

def docstring_for_should
"is nil"
end

def docstring_for_should_not
"is not nil"
end
end end


class Be < BaseMatcher class Be < BaseMatcher
Expand All @@ -62,6 +86,14 @@ def failure_message_for_should_not
"expected #{@actual.inspect} to evaluate to false" "expected #{@actual.inspect} to evaluate to false"
end end


def docstring_for_should
"is"
end

def docstring_for_should_not
"is not"
end

[:==, :<, :<=, :>=, :>, :===].each do |operator| [:==, :<, :<=, :>=, :>, :===].each do |operator|
define_method operator do |operand| define_method operator do |operand|
BeComparedTo.new(operand, operator) BeComparedTo.new(operand, operator)
Expand Down Expand Up @@ -117,8 +149,12 @@ def failure_message_for_should_not
"It might be more clearly expressed in the positive?") "It might be more clearly expressed in the positive?")
end end


def description def docstring_for_should
"be #{@operator} #{expected_to_sentence}#{args_to_sentence}" "is #{@operator} #{expected_to_sentence}#{args_to_sentence}"
end

def docstring_for_should_not
"is not #{@operator} #{expected_to_sentence}#{args_to_sentence}"
end end
end end


Expand Down Expand Up @@ -152,8 +188,12 @@ def failure_message_for_should_not
"expected #{predicate}#{args_to_s} to return false, got #{@result.inspect}" "expected #{predicate}#{args_to_s} to return false, got #{@result.inspect}"
end end


def description def docstring_for_should
"#{prefix_to_sentence}#{expected_to_sentence}#{args_to_sentence}" "is #{expected_to_sentence}#{args_to_sentence}"
end

def docstring_for_should_not
"is not #{expected_to_sentence}#{args_to_sentence}"
end end


private private
Expand Down
18 changes: 13 additions & 5 deletions lib/rspec/matchers/built_in/be_within.rb
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ def initialize(delta)


def matches?(actual) def matches?(actual)
@actual = actual @actual = actual
raise needs_expected unless defined? @expected raise needs_expected unless defined? @expected
raise needs_subtractable unless @actual.respond_to? :- raise needs_subtractable unless @actual.respond_to? :-
(@actual - @expected).abs <= @delta (@actual - @expected).abs <= @delta
end end
Expand All @@ -20,19 +20,27 @@ def of(expected)
end end


def failure_message_for_should def failure_message_for_should
"expected #{@actual} to #{description}" "expected #{@actual} to be #{docstring}"
end end


def failure_message_for_should_not def failure_message_for_should_not
"expected #{@actual} not to #{description}" "expected #{@actual} not to be #{docstring}"
end end


def description def docstring_for_should
"be within #{@delta} of #{@expected}" "is #{docstring}"
end

def docstring_for_should_not
"is not #{docstring}"
end end


private private


def docstring
"within #{@delta} of #{@expected}"
end

def needs_subtractable def needs_subtractable
ArgumentError.new "The actual value (#{@actual.inspect}) must respond to `-`" ArgumentError.new "The actual value (#{@actual.inspect}) must respond to `-`"
end end
Expand Down
10 changes: 7 additions & 3 deletions lib/rspec/matchers/built_in/change.rb
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ def by_at_least(minimum)
def by_at_most(maximum) def by_at_most(maximum)
@maximum = maximum @maximum = maximum
self self
end end


def to(to) def to(to)
@eval_after = true @eval_after = true
Expand All @@ -86,8 +86,12 @@ def from (before)
self self
end end


def description def docstring_for_should
"change ##{message}" "changes ##{message}"
end

def docstring_for_should_not
"does not change ##{message}"
end end


private private
Expand Down
9 changes: 8 additions & 1 deletion lib/rspec/matchers/built_in/eq.rb
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -14,9 +14,16 @@ def failure_message_for_should_not
"\nexpected: value != #{expected.inspect}\n got: #{actual.inspect}\n\n(compared using ==)\n" "\nexpected: value != #{expected.inspect}\n got: #{actual.inspect}\n\n(compared using ==)\n"
end end


def docstring_for_should
"eq #{expected.inspect}"
end

def docstring_for_should_not
"does not eq #{expected.inspect}"
end

def diffable?; true; end def diffable?; true; end
end end
end end
end end
end end

8 changes: 8 additions & 0 deletions lib/rspec/matchers/built_in/eql.rb
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -14,6 +14,14 @@ def failure_message_for_should_not
"\nexpected: value != #{expected.inspect}\n got: #{actual.inspect}\n\n(compared using eql?)\n" "\nexpected: value != #{expected.inspect}\n got: #{actual.inspect}\n\n(compared using eql?)\n"
end end


def docstring_for_should
"eql #{expected.inspect}"
end

def docstring_for_should_not
"does not eql #{expected.inspect}"
end

def diffable? def diffable?
true true
end end
Expand Down
Loading

0 comments on commit 2f0d105

Please sign in to comment.