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

add chained expectation descriptions to the default descriptions of custom matchers with fluent interfaces #600

Merged
merged 2 commits into from Jul 23, 2014

Conversation

Projects
None yet
3 participants
@oveddan
Contributor

oveddan commented Jul 9, 2014

this fixes #532

add chained expectation descriptions to the default descriptions of c…
…ustom matchers with fluent interfaces

this fixes #532
@@ -272,6 +275,15 @@ def supports_block_expectations?
def expects_call_stack_jump?
false
end
private

This comment has been minimized.

@JonRowe

JonRowe Jul 9, 2014

Member

This, and the following method needs dedenting in accordance with our style, and it needs a new line after it.

@@ -272,6 +275,15 @@ def supports_block_expectations?
def expects_call_stack_jump?
false
end
private
# gets chained method invokations, combined into a sentance

This comment has been minimized.

@JonRowe

JonRowe Jul 9, 2014

Member

You don't need this comment, the rest have comments for YARD purposes.

@@ -178,6 +179,8 @@ def supports_block_expectations
def chain(name, &definition)
define_user_override(name, definition) do |*args, &block|
super(*args, &block)
@chained_method_invokations ||= []

This comment has been minimized.

@JonRowe

JonRowe Jul 9, 2014

Member

I think if you move this into the matcher initialiser you won't need this or the defined? check below.

This comment has been minimized.

@oveddan

oveddan Jul 9, 2014

Contributor

Yes this could be done - but then this variable is created every time the matcher is used, regardless of if the chained methods are used or not. Not sure if there are performance overhead concerns with this. What do you think?

This comment has been minimized.

@JonRowe

JonRowe Jul 9, 2014

Member

I quickly benchmarked it, for me adding an ivar containing an array in an initialiser takes an additional 0.2μs seconds (averaged from 10000), whilst checking if an ivar is defined takes 0.1μs, as does ||=.

In short, I'm not concerned about the performance impact, and it's neater coder.

This comment has been minimized.

@oveddan

oveddan Jul 9, 2014

Contributor

Ok cool will move it into the initializer.

This comment has been minimized.

@myronmarston

myronmarston Jul 9, 2014

Member

I've always seen this spelled "invocations", not "invokations".

(And I agree with moving it to the initialize method).

This comment has been minimized.

@JonRowe

JonRowe Jul 9, 2014

Member

(👍 to invocations)

This comment has been minimized.

@oveddan

oveddan Jul 17, 2014

Contributor

@JonRowe where exactly should this initialiser sit?

This comment has been minimized.

@JonRowe

JonRowe Jul 17, 2014

Member

There's an existing initialiser lib/rspec/matchers/dsl.rb#L323

This comment has been minimized.

@oveddan

oveddan Jul 17, 2014

Contributor

Thanks!

end
# The default failure message for positive expectations.
def failure_message
"expected #{actual.inspect} to #{name_to_sentence}#{to_sentence expected}"
"expected #{actual.inspect} to #{description}"

This comment has been minimized.

@JonRowe

JonRowe Jul 9, 2014

Member

@myronmarston do you know why we didn't do this before?

This comment has been minimized.

@JonRowe

JonRowe Jul 9, 2014

Member

I'm thinking because a user can override description but we don't necessarily want that to be reflected here?

This comment has been minimized.

@myronmarston

myronmarston Jul 9, 2014

Member

No idea. I bet it's just that no one noticed the fact that it could be simplified like this. I noticed at one point that this could be done in BaseMatcher in b030ec5. I didn't think to change it here though. I'm in favor of this change, though.

# gets chained method invokations, combined into a sentance
def chained_method_invokation_sentences
defined? @chained_method_invokations and
@chained_method_invokations.map { |chained_method_invokation|

This comment has been minimized.

@JonRowe

JonRowe Jul 9, 2014

Member

Please use do/end here to be consistent with our style

description do
super() + " equal to #{@expected_value}"
end

This comment has been minimized.

@JonRowe

JonRowe Jul 9, 2014

Member

Hmm...

This comment has been minimized.

@oveddan

oveddan Jul 9, 2014

Contributor

The description override is no longer necessary when the chained method descriptions can get automatically generated. Leaving it in results in provides undesired behavior, resulting in a failure message:
"raise an error with attribute :foo equal to 3 equal to 3"

This comment has been minimized.

@myronmarston

myronmarston Jul 9, 2014

Member

Hmm, this makes me realize that this change is going to make sub-optimal descriptions (and potentially, failure messages) for existing custom matchers that use chain and were overriding the description to compensate for hte fact that the chained part wasn't being included.

Is there a way we can avoid this problem? (I can't think of a solution that I like).

This comment has been minimized.

@oveddan

oveddan Jul 9, 2014

Contributor

@myronmarston The default description can check if a custom description is provided before appending the chained "clauses"

This comment has been minimized.

@myronmarston

myronmarston Jul 9, 2014

Member

Hmm, that's certainly one idea. I'm concerned that it would create confusion when the user starts with this:

RSpec::Matchers.define :some_cool_matcher do
  chain(:foo) { }
  match { }
end

...and then, after noticing the description and thinking they want to append a bit to what it was already producing, they change it to:

RSpec::Matchers.define :some_cool_matcher do
  chain(:foo) { }
  match { }
  description do
    super() + " my extra"
  end
end

...expecting the super() there to get the same description that was already being produced. It would be confusing for it to not include the chained bits when description did previously.

This comment has been minimized.

@myronmarston

myronmarston Jul 17, 2014

Member

As far as the configuration of this value, the natural inclination would be for it to be in RSpec::Expectations::Configuration since its value is consumed in this library.

Correct. That's where the config option belongs.

But if rspec-core is generating spec_helper, and is setting a default value for this, shouldn't the config for this be located in the Core configuration, even though it's not consumed in that library? That feels a bit weird, right?

I don't think it feels weird. We already have lines in the generated spec_helper that set rspec-expectations and rspec-mocks config settings.

This comment has been minimized.

@myronmarston

myronmarston Jul 17, 2014

Member

Do you think this is a likely case? If so, and the value is configured, the libraries with the out of date rspec versions would raise an error since they would not be aware of that configuration method.

Sure...that's just like any other new API that we add in a minor release. If gems choose to use the new API, consuming projects have to be on or above the release that added the API. That's the way this always works.

This comment has been minimized.

@oveddan

oveddan Jul 22, 2014

Contributor

Thanks @myronmarston @JonRowe for your responsiveness and attention to detail, making rspec the best testing library our there.

Quick question re:naming conventions. Should test names and context descriptions be in single or double quotes (it's not consistent in the test suite suite now)?

This comment has been minimized.

@myronmarston

myronmarston Jul 22, 2014

Member

It's inconsistent because we have no preference :).

This comment has been minimized.

@JonRowe

JonRowe Jul 22, 2014

Member

I normally go double but it's just a personal preference.

@JonRowe

This comment has been minimized.

Member

JonRowe commented Jul 9, 2014

A good start, this is currently failing because of our Rubocop settings, can you correct the styling to match please?

@oveddan

This comment has been minimized.

Contributor

oveddan commented Jul 9, 2014

@JonRowe thanks for all of your comments, sure will do.

@@ -249,17 +252,17 @@ def diffable?
# The default description.
def description
"#{name_to_sentence}#{to_sentence expected}"
"#{name_to_sentence}#{to_sentence expected}#{chained_method_invokation_sentences}"

This comment has been minimized.

@myronmarston

myronmarston Jul 9, 2014

Member

Same here -- I think this should be invocation, not invokation?

Also, they are not really sentences (as that implies --at least to me -- that it should have a priod/full stop for each). Maybe clauses is a better term to use here?

defined? @chained_method_invokations and
@chained_method_invokations.map { |chained_method_invokation|
"#{split_words chained_method_invokation[0]}#{to_sentence chained_method_invokation[1]}"
}.join(' ').insert(0, ' ')

This comment has been minimized.

@myronmarston

myronmarston Jul 9, 2014

Member

Your map logic can be simplified by using destructing for the block args:

@chained_method_invokations.map do |(method_name, args)|
  split_words(method_name) + to_sentence(args)
end.join(' ').insert(0, ' ')
it "provides a default negative expectation failure message that does not include the any of the chained matchers's descriptions" do
matcher.matches?(10)
expect(matcher.failure_message_when_negated).to eq "expected 10 not to be bigger than 5"

This comment has been minimized.

@myronmarston

myronmarston Jul 9, 2014

Member

Rather than calling the matcher protocol directly in these specs I think it's better to just use the matcher and wrap it in an expect { }.to fail_with(msg) expectation.

This comment has been minimized.

@oveddan

oveddan Jul 17, 2014

Contributor

@myronmarston I tried this, by changing to:

it "provides a default positive expectation failure message that does not include any of the chained matchers' descriptions" do
    expect { matcher.matches?(2) }.to fail_with 'expected 2 to be bigger than 5'
end

but getting the failed tests:

Failure/Error: expect { matcher.matches?(2) }.to fail_with 'expected 2 to be bigger than 5'
expected RSpec::Expectations::ExpectationNotMetError with "expected 2 to be bigger than 5" but nothing was raised

What am I doing wrong?

This comment has been minimized.

@JonRowe

JonRowe Jul 17, 2014

Member

Swap matcher.matches?(2) for expect(2).to matcher

This comment has been minimized.

@oveddan

oveddan Jul 17, 2014

Contributor

That worked thx

Scenario: include_chain_clauses_in_custom_matcher_descriptions configured to true, and chained method with argument
Given a file named "between_spec.rb" with:
"""ruby
RSpec::Matchers.configuration.include_chain_clauses_in_custom_matcher_descriptions = true

This comment has been minimized.

@myronmarston

myronmarston Jul 22, 2014

Member

I think most users will configure this via the rspec-core config API:

RSpec.configure do |config|
  config.expect_with :rspec do |c|
    c.include_chain_clauses_in_custom_matcher_descriptions = true
  end
end

...and I'd prefer that was used here as well.

This comment has been minimized.

@oveddan

oveddan Jul 22, 2014

Contributor

I was not sure of how to access the expectation configuration - thanks for showing me.

My attempts at doing:

RSpec.configure do |config|
  c.include_chain_clauses_in_custom_matcher_descriptions = true
end

were raising an error, but now I see the correct way of accessing the expectation configuration.

This comment has been minimized.

@oveddan

oveddan Jul 23, 2014

Contributor

@myronmarston Cannot figure out how to cleanly set config this way, then during cleanup revert it back to its default . I can't seem to find a spec example where configuration is modified as in the example you sent and then the behavior of code outside of Configuration is affected. Can you point me to one?

This comment has been minimized.

@oveddan

oveddan Jul 23, 2014

Contributor

This is what I currently have but it seems overly verbose:

context "with include_chain_clauses_in_custom_matcher_descriptions configured to true" do
  before do
    RSpec.configure do |config|
      config.expect_with :rspec do |c|
        c.include_chain_clauses_in_custom_matcher_descriptions = true
      end
    end
  end

  after do
    RSpec.configure do |config|
      config.expect_with :rspec do |c|
        c.include_chain_clauses_in_custom_matcher_descriptions = nil
      end
    end
  end

  it "provides a default description that does includes the chained matchers' descriptions" do

    expect(matcher.and_divisible_by(10).description).to eq 'be even and divisible by 10 and divisible by 10'
  end
end

is there a better way?

This comment has been minimized.

@oveddan

oveddan Jul 23, 2014

Contributor

Disregard that question - in the cucumber feature it's fine.

@myronmarston

This comment has been minimized.

Member

myronmarston commented Jul 22, 2014

I noticed you addressed our feedback @oveddan...thanks! In the future please ping us as we don't get any notification that new commits have been pushed. I just happened to check this issue. I'm going to review your updates now.

attr_writer :include_chain_clauses_in_custom_matcher_descriptions
def include_chain_clauses_in_custom_matcher_descriptions
@include_chain_clauses_in_custom_matcher_descriptions ||= false
end

This comment has been minimized.

@myronmarston

myronmarston Jul 22, 2014

Member
  • Would be nice if the reader method had a question mark to make it clear it's a boolean predicate.
  • The YARD docs don't mention that it should be a Boolean -- would be nice if they would. Currently the rendered docs say this attribute is an object.
  • I think that "chained expectations" is a confusing phrase -- I would consider expect(x).to matcher_1.and_matcher_2.and matcher_3 to be "chained expectations" (as this chains together multiple expectations). Instead, I'd phrase this like:
# Sets or gets if custom matcher descriptions and failure messages
# should include clauses from methods defined using `chain`. It is
# false by default for backwards compatibility.

This comment has been minimized.

@oveddan

oveddan Jul 22, 2014

Contributor

Agreed regarding the ? - I just did not see it in any other configuration options so tried to keep it consistent, but ? is definitely the way to go.

# @private
def reset_include_chain_clauses_in_custom_matcher_descriptions_to_default
@include_chain_clauses_in_custom_matcher_descriptions = nil
end

This comment has been minimized.

@myronmarston

myronmarston Jul 22, 2014

Member

Given that this isn't intended to be a public API and is only used by our tests (and actually, I have some suggestions for the tests that will render it obsolete, I think), I'd prefer that we not have this method at all.

This comment has been minimized.

@oveddan

oveddan Jul 22, 2014

Contributor

@myronmarston totally agreed, it was a smell for me to include a method that is just going to be consumed by tests here. The decision to include it was influenced by "reset_syntaxes_to_default" - is that used anywhere besides the tests?

You are right tho, reset_include_chain_clauses_in_custom_matcher_descriptions_to_default can be eliminated by just setting the value to nil.

This comment has been minimized.

@myronmarston

myronmarston Jul 22, 2014

Member

The decision to include it was influenced by "reset_syntaxes_to_default" - is that used anywhere besides the tests?

It's used here:

configuration.reset_syntaxes_to_default

Also, the "default" for the syntaxes is more involved because we issue a deprecation warning if users use the old :should syntax without explicitly configuring it -- so this method helps manage that.

# for you. If the expectation is used, and
# RSpec::Matchers.configuration.include_chain_clauses_in_custom_matcher_descriptions
# is true, it also adds the chained expectation's clause to the
# default description.

This comment has been minimized.

@myronmarston

myronmarston Jul 22, 2014

Member

I don't think "expectation" is the right term here. (In this library, we consider an expectation to be a whole expect(...).to matcher expression). Also, RSpec::Matchers.configuration isn't the normal API we recommend using for setting config (see our docs) -- it's really just there because rspec-core needs it for it to work with expect_with(:rspec) { }. I think I'd rephrase this as something like:

# If the method is called, and the `include_chain_clauses_in_custom_matcher_descriptions`
# config option has been enabled, then the default `description` and `failure_message`
# will include a corresponding English phrase based on the method name and arguments.
@@ -178,6 +181,9 @@ def supports_block_expectations
def chain(name, &definition)
define_user_override(name, definition) do |*args, &block|
super(*args, &block)
if RSpec::Matchers.configuration.include_chain_clauses_in_custom_matcher_descriptions
@chained_method_clauses.push([name.to_s, args])

This comment has been minimized.

@myronmarston

myronmarston Jul 22, 2014

Member

name.to_s can just be name, I believe.

"#{split_words(method_name)}#{to_sentence(method_args)}"
end.join(' ').insert(0, ' ')
end
end

This comment has been minimized.

@myronmarston

myronmarston Jul 22, 2014

Member

Maybe this is just the way I instinctively structure code, but it kinda makes more sense to me to check the config option in this method (and always push the name/args on the @chained_method_clauses list when the chain method is invoked), since what the config option is about is whether or not the clauses are included in the description, not whether or not the implementation keeps track of the chain method invocations internally. It has a side benefit of operating in a bit less surprising manner if users do something crazy like:

RSpec::Expectations.include_chain_clauses_in_custom_matcher_descriptions = false
matcher = be_bigger_than(4).and_smaller_than(6)
RSpec::Expectations.include_chain_clauses_in_custom_matcher_descriptions = true
matcher.description

...not that we'd officially support doing that, but if a crazy user tries that it feels more "correct" to me for the description to include the chained clauses. Putting this together with my preference for guard clauses (and the fact that you can simplify away the .empty? check), and this method could be:

def chained_method_clause_sentences
  return '' if Expectations.configuration.include_chain_clauses_in_custom_matcher_descriptions?

  @chained_method_clauses.map do |(method_name, method_args)|
    " #{split_words(method_name)}#{to_sentence(method_args)}"
  end.join
end
  • Given this method is meant to return a string, I think it's better to return the blank string rather than nil.
  • I realized that you don't need to bother with making the empty? a special case if you remove the ' ' arg for join and the extra insert call, instead putting the prefixing space directly in the interpolated string in the map block. Much simpler!

This comment has been minimized.

@oveddan

oveddan Jul 22, 2014

Contributor

Yes that makes sense, especially since the decision to include the chained clauses can be done when description is invoked.

RSpec::Matchers.configuration.include_chain_clauses_in_custom_matcher_descriptions = nil
expect(RSpec::Matchers.configuration.include_chain_clauses_in_custom_matcher_descriptions).to be false
end
end

This comment has been minimized.

@myronmarston

myronmarston Jul 22, 2014

Member

Rather than using reset_include_chain_clauses_in_custom_matcher_descriptions_to_default to put the global config instance into its default state, I'd recommend simply using the new config instance already available to you. Since it's a brand new instance, there's no need to call reset_include_chain_clauses_in_custom_matcher_descriptions_to_default (or for that method to even exist to support this kind of use case).

This comment has been minimized.

@oveddan

oveddan Jul 22, 2014

Contributor

Yes, that is cleaner - but for some reason, when trying to set "include_chain_clauses_in_custom_matcher_descriptions" on the rspec core configuration, I got an error that the method was not defined. How does rspec core configuration know about the options available from RSpec::Expectation configuration?

This comment has been minimized.

@myronmarston

myronmarston Jul 22, 2014

Member

I think you answered your own question via the comment on the cuke above?

This comment has been minimized.

@oveddan

oveddan Jul 22, 2014

Contributor

That I did :)

This comment has been minimized.

@oveddan

oveddan Jul 22, 2014

Contributor

The cucumber feature should be updated as well to reflect this.

it "can be set back to false" do
RSpec::Matchers.configuration.include_chain_clauses_in_custom_matcher_descriptions = true
RSpec::Matchers.configuration.include_chain_clauses_in_custom_matcher_descriptions = nil

This comment has been minimized.

@myronmarston

myronmarston Jul 22, 2014

Member

It seems odd to set it to nil here as a way of demonstrating it can be set back to false. Why not set it to false?

This comment has been minimized.

@oveddan

oveddan Jul 22, 2014

Contributor

Yes I was debating whether to set it to nil or false, but setting to nil seemed to show that you can set it back to it's default value. false may be clearer for this test's purpose, since the test is about setting it to false

RSpec::Matchers.configuration.reset_include_chain_clauses_in_custom_matcher_descriptions_to_default
expect(RSpec::Matchers.configuration.include_chain_clauses_in_custom_matcher_descriptions).to be false
end
end

This comment has been minimized.

@myronmarston

myronmarston Jul 22, 2014

Member

This spec can be deleted since we can do away with reset_include_chain_clauses_in_custom_matcher_descriptions_to_default entirely :).

before do
RSpec::Matchers.configuration.reset_include_chain_clauses_in_custom_matcher_descriptions_to_default
end

This comment has been minimized.

@myronmarston

myronmarston Jul 22, 2014

Member

While your original change to the an_error_with_attribute custom matcher above prompted the creation of this config option (as it helped us realize that this would be a breaking change!), at this point I'd prefer us to leverage the new feature internally rather than keeping our description override above and calling the reset method here. We can set the config option to true in spec_helper.rb so it's on for our specs.

This comment has been minimized.

@oveddan

oveddan Jul 22, 2014

Contributor

Agreed, and another thing I was pondering as well. Should we update the tests to reflect the new way descriptions are generated, or let them show the original way, where you have to opt in to get the chained clauses? The former is probably better as to not encourage people to use the old behavior.

This comment has been minimized.

@myronmarston

myronmarston Jul 22, 2014

Member

I'd like to have it show the new way. The old way is just there for backwards compatibility, not that it's in any way better.

chain :but_divisible_by do |divisor|
@divisor = divisor
end

This comment has been minimized.

@myronmarston

myronmarston Jul 22, 2014

Member

I think that "but" is the wrong conjunction here (as it normally indicates an opposing/contrasting thing). I think expect(x).to be_bigger_than(3).and_smaller_than(5).and_divisible_by(2) (with and instead of but) reads better.

This comment has been minimized.

@oveddan

oveddan Jul 22, 2014

Contributor

agreed

@divisor = divisor
end
private

This comment has been minimized.

@myronmarston

myronmarston Jul 22, 2014

Member

Not a merge blocker, but we prefer dedented private:

class Foo
  def some_public_method
  end

private

  def some_private_method
  end
end
it "provides a default negative expectation failure message that includes the chained matchers' failures" do
expect { expect(9).to_not match }.to fail_with 'expected 9 not to be bigger than 5 but smaller than 10 but divisible by 3'
end
end

This comment has been minimized.

@myronmarston

myronmarston Jul 22, 2014

Member

One other spec that would be useful is to show what order the clauses are in when there are multiple of them. The clauses should be in invocation order, not chain definition order. (One can imagine an implementation that does the latter that would pass your current specs). To spec that, you'd just use a matcher like matcher.and_divisible_by(3).and_smaller_than(10) (the opposite order of the definition of and_smaller_than vs and_divisible_by) and show the description that results.

context "with include_chain_clauses_in_custom_matcher_descriptions configured to true" do
it "provides a default description that does includes the chained matchers' descriptions" do
RSpec::Matchers.configuration.include_chain_clauses_in_custom_matcher_descriptions = true

This comment has been minimized.

@myronmarston

myronmarston Jul 22, 2014

Member

As I mentioned elsewhere, we consider Expectations.configuration to be the main way to access the config object (rather than Matchers.configuration) so we'd prefer to see that used everywhere.

(actual > first) && (actual < @second)
end
chain :but_smaller_than do |second|

This comment has been minimized.

@myronmarston

myronmarston Jul 22, 2014

Member

As I commented below, I think and_smaller_than is a better name for this than but_smaller_than.

@myronmarston

This comment has been minimized.

Member

myronmarston commented Jul 22, 2014

OK, @oveddan, this is looking really good. I left some more feedback. I realize this PR has dragged on so I'm happy to apply the feedback I left as part of merging if you'd prefer that over taking the time to address it yourself. Or you can address the feedback yourself, of course. Either way is fine, but we definitely want to get this in 3.1 :).

@oveddan

This comment has been minimized.

Contributor

oveddan commented Jul 22, 2014

@myronmarston Thanks again for all the attention to detail. Feel free to take on the changes if you have the time, I could do the changes tonight (EST) but if you have time now and want to, go ahead.

@myronmarston

This comment has been minimized.

Member

myronmarston commented Jul 22, 2014

@myronmarston Thanks again for all the attention to detail. Feel free to take on the changes if you have the time, I could do the changes tonight (EST) but if you have time now and want to, go ahead.

I'll gladly let you continue with this :).

@oveddan

This comment has been minimized.

Contributor

oveddan commented Jul 22, 2014

Cool, will do

@oveddan

This comment has been minimized.

Contributor

oveddan commented Jul 23, 2014

@myronmarston @JonRowe pushed an update, please review. Thanks!

@oveddan

This comment has been minimized.

Contributor

oveddan commented Jul 23, 2014

(I'm fixing the 1988.1 build failure)

Added RSpec::Matchers.configuration.include_chain_clauses_in_custom_m…
…atcher_descriptions that

when set to true, custom matcher descriptions and failure messages will include clauses from
methods defined using `chain`.

Fixed some code to match the naming conventions

Fixes #532

myronmarston added a commit that referenced this pull request Jul 23, 2014

Merge pull request #600 from oveddan/chained-matcher-descriptions
add chained expectation descriptions to the default descriptions of custom matchers with fluent interfaces

@myronmarston myronmarston merged commit 0ba1f39 into rspec:master Jul 23, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@myronmarston

This comment has been minimized.

Member

myronmarston commented Jul 23, 2014

Thanks, @oveddan!

myronmarston added a commit that referenced this pull request Jul 23, 2014

@myronmarston myronmarston referenced this pull request Jul 23, 2014

Merged

Pr 600 followups #609

myronmarston added a commit that referenced this pull request Jul 23, 2014

@oveddan

This comment has been minimized.

Contributor

oveddan commented Jul 23, 2014

@myronmarston should we create an issue in rspec core to have the rspec config generator make this option true for new projects?

@myronmarston

This comment has been minimized.

Member

myronmarston commented Jul 23, 2014

Already done: rspec/rspec-core#1645.

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