Skip to content
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

Implement exclusive mode in be_between #412

Merged
merged 9 commits into from Jan 8, 2014

Conversation

@pedrogimenez
Copy link
Contributor

@pedrogimenez pedrogimenez commented Jan 6, 2014

No description provided.

# You can chain any of the following off of the end to specify details
# about the change:
#
# * `exclusive`

This comment has been minimized.

@myronmarston

myronmarston Jan 6, 2014
Member

I know you copied this from the change matcher, but it doesn't make much sense here since exclusive is the only modifier you can chain off of the matcher. Here's what I'd do instead for this whole section

# By default, `be_between` is inclusive (i.e. passes when given either the max or min value),
# but you can make it `exclusive` by chaining that off the matcher.

This comment has been minimized.

@pedrogimenez

pedrogimenez Jan 6, 2014
Author Contributor

You're right. Thanks for the feedback, I'll change it.

@@ -21,7 +25,7 @@ def description
"be between #{@min.inspect} and #{@max.inspect} (inclusive)"
end

private
private

This comment has been minimized.

@myronmarston

myronmarston Jan 6, 2014
Member

Please leave this indentation -- we prefer keeping private indented at the same level as class.

This comment has been minimized.

@pedrogimenez

pedrogimenez Jan 6, 2014
Author Contributor

Ok, I'll change it 👍

This comment has been minimized.

@sferik

sferik Jan 6, 2014
Contributor

@myronmarston You can use rubocop to programmatically enforce style rules like this, as I have done in all my projects. This particular rule (or "cop", as it’s called in RuboCop parlance) is called:

AccessModifierIndentation:
  EnforcedStyle: outdent

This comment has been minimized.

@myronmarston

myronmarston Jan 6, 2014
Member

Good to know. Enforcing style isn't high on my priorities right now but I'll look into rubocop at some point.

This comment has been minimized.

@sferik

sferik Jan 6, 2014
Contributor

Would you be interested in a pull request that adds RuboCop? Is there any existing style guide for RSpec for me to start from or should I just try to glean the preferred style from the existing code?

This comment has been minimized.

@myronmarston

myronmarston Jan 6, 2014
Member

Sure! I think our conventions are pretty typical for the ruby community, but a couple things I can think of:

  • The private indentation thing we talked about here.
  • We don't use seattle style for method defs, e.g. def foo(a, b) rather than def foo a, b.

I'm blanking on other stuff but I'm sure there's more.

It would be nice if common config for this could go in rspec-support so we don't have to maintain the same set of config across multiple repos.

BTW, I've been thinking of looking into integrating yardstick as well, which I see that you use. For RSpec 3 I would like 100% of our public APIs documented and everything that's not public documented as not being public. It seems like a good tool to help with that. Seeing as you have the latest commit on that repo, it looks like you have experience there, so if you want to help get that set up for us that would be superb :).

This comment has been minimized.

@yujinakayama

yujinakayama Jan 7, 2014
Member

  • The private indentation thing we talked about here.
  • We don't use seattle style for method defs, e.g. def foo(a, b) rather than def foo a, b.

These are already supported in RuboCop.
Let me know when you need a help with RuboCop. :)

"be between #{@min.inspect} and #{@max.inspect} (exclusive)"
end

private

This comment has been minimized.

@myronmarston

myronmarston Jan 6, 2014
Member

We prefer having this de-indented two spaces (at the same level as class). (Not a merge blocker though -- just bringing it up for consistency with the above class)

end

def compare
@actual > @min and @actual < @max

This comment has been minimized.

@myronmarston

myronmarston Jan 6, 2014
Member

Seeing this implementation (which is the only way you can can do it -- since between? doesn't let you make it exclusive), makes me think that maybe we should change the BeBetween matcher to use >= and <= rather than between? for a few reasons:

  • Someone can implement between? on their object in a way that's exclusive (rather than inclusive), which would make the matcher not be inclusive as documented.
  • There are ruby objects that are comparable with <, <=, > and >= but do not respond to between?. Those objects will work with this exclusive matcher but not with the inclusive one. For example, classes: expect(Integer).to be_between(Object, Fixnum) -- they do not include Comparable, but do implement the comparison operators, and while it's a little odd, it's reasonable to say that Integer is between Object and Fixnum in the inheritance hierarchy.
  • Using the comparison operators would allow us to reduce duplication: we could have a single matcher class, with @less_than_operator and @greater_than_opeator ivars that default to <= and >= but get changed to < and > when exclusive is called. (The (inclusive) and (exclusive) bit in the description could be extracted into a variable as well).

@sferik -- as the original author of the BeBetween authors, what are your thoughts?

This comment has been minimized.

@pedrogimenez

pedrogimenez Jan 6, 2014
Author Contributor

+1.

This comment has been minimized.

@sferik

sferik Jan 6, 2014
Contributor

This sounds good to me.

expect {
expect([nil, 1]).to include(a_value_between(2, 4).exclusive, a_nil_value)
}.to fail_with("expected [nil, 1] to include (a value between 2 and 4 (exclusive)) and (a nil value)")
end

This comment has been minimized.

@myronmarston

myronmarston Jan 6, 2014
Member

Given the fact that the behavior of be_between vs be_between(...).exclusive is identical except for the failure message and the behavior at the min and max values, I think this is a good use case for shared example groups:

shared_examples_for "be_between" do |between_type|
  it 'passes if target is < max and > min' do
    expect(5).to matcher(1, 10)
  end

  it 'fails if target is > max' do
    expect {
      expect(11).to matcher(1, 10)
    }.to fail_with("expected 11 to be between 1 and 10 (#{between_type})")
  end

   # etc
end

describe "be_between (inclusive)" do
  it_behaves_like "be between", :inclusive do
    def matcher(min, max)
      be_between(min, max)
    end
  end

  # specs unique to inclusive would go here
end

describe "be_between (exclusive)" do
  it_behaves_like "be between", :exclusive do
    def matcher(min, max)
      be_between(min, max).exlusive
    end
  end

  # specs unique to exclusive would go here
end

This comment has been minimized.

@pedrogimenez

pedrogimenez Jan 6, 2014
Author Contributor

Yep, I thought so.

@pedrogimenez
Copy link
Contributor Author

@pedrogimenez pedrogimenez commented Jan 7, 2014

Hey, @myronmarston and @sferik could you take a look? ✌️

end

def compare
@actual.send(@greater_than_operator, @min) and @actual.send(@less_than_operator, @max)

This comment has been minimized.

@myronmarston

myronmarston Jan 7, 2014
Member

It's a good idea to use __send__ instead of send. send can be defined on user domain objects (e.g. Email#send). Ruby will warn if users redefine __send__.

This comment has been minimized.

@myronmarston

myronmarston Jan 7, 2014
Member

@sferik / @yujinakayama - is there a rubocop setting that enforces __send__ over send?

This comment has been minimized.

@pedrogimenez

pedrogimenez Jan 7, 2014
Author Contributor

Oh, I didn't know that. Thanks!

This comment has been minimized.

@sferik

sferik Jan 7, 2014
Contributor

@myronmarston Not presently, but it would be trivial to add a cop that enforces that rule. You could also just request the feature.

This comment has been minimized.

@yujinakayama

yujinakayama Jan 7, 2014
Member

@myronmarston No.

Static code analysis does not provide runtime class information, so it's hard to guess the intent of a #send:

  • As you said, it might be Email#send.
  • If the receiver object is a known class (i.e. defined in the same project), #send can be used safely.

This comment has been minimized.

@myronmarston

myronmarston Jan 7, 2014
Member

@sferik -- I'll probably formally request it once I start using rubocop. I feel weird making a feature request to a project I haven't tried to use yet.

@yujinakayama -- it makes sense to me that rubocop wouldn't enforce __send__ over send by default, but for libraries like RSpec, I think it would be useful. I'd be fine with it enforcing it statically (e.g. not allowing send at all, even for classes that define it) since we won't ever define it as a method on anything and always want __send__ to be used.

This comment has been minimized.

@yujinakayama

yujinakayama Jan 7, 2014
Member

@myronmarston Fair enough. I think adding these cops would make sense:

  • A cop that checks for redefinition of #send (enabled by default)
  • Another cop that checks for use of #send (disabled by default)

This comment has been minimized.

@myronmarston

myronmarston Jan 7, 2014
Member

Sounds good to me :).

end

private

def comparable?
@actual.respond_to?(:between?)
@actual.respond_to?(@less_than_operator) and @actual.respond_to?(@greater_than_operator)

This comment has been minimized.

@myronmarston

myronmarston Jan 7, 2014
Member

I prefer limiting and and or to only control-flow situations, using && and || for boolean expressions.

http://devblog.avdi.org/2010/08/02/using-and-and-or-in-ruby/

Do you mind changing the boolean and here (and in compare and maches?) to use &&?

This comment has been minimized.

@pedrogimenez

pedrogimenez Jan 7, 2014
Author Contributor

Ok, I'll change it!

@less_than_operator = :<
@greater_than_operator = :>
@mode = :exclusive
self
end

This comment has been minimized.

@myronmarston

myronmarston Jan 7, 2014
Member

I'm thinking that it would be good to add an inclusive method as well. be_between would remain inclusive by default; the method would just be a way for users to make that explicit if/when they want to. You could move the ivar assignments out of initialize into inclusive, and then call inclusive from initialize -- that would have a nice side benefit of telling a more clear story as well.

This comment has been minimized.

@pedrogimenez

pedrogimenez Jan 7, 2014
Author Contributor

Sounds good. It's more idiomatic 👍

end
end

describe "composing with other matchers" do
shared_examples_for "composing with other matchers" do |mode|

This comment has been minimized.

@myronmarston

myronmarston Jan 7, 2014
Member

You're registering these shared example groups at the top level. That means there can't be any other shared example groups in the project named "composing with other matchers" -- but that's a pretty general name. Instead, please do this:

module RSpec
  module Matchers
    describe BeBetween do
      shared_examples_for "composing with other matchers" do |mode|
      end

      describe "expect(...).to be_between(min, max) (inclusive)" do
        it_behaves_like "composing with other matchers"
      end
    end
  end
end

By putting the shared example definitions within an example group, it limits their scope to just that example group, so they can't collide with shared example groups declared elsewhere.

end

it "is inclusive" do
expect(10).to be_between(1, 10)

This comment has been minimized.

@myronmarston

myronmarston Jan 7, 2014
Member

Would be nice for this to show both ends:

expect(10).to be_between(1, 10)
expect(1).to be_between(1, 10)
end

it "is exclusive" do
expect(10).not_to be_between(1, 10).exclusive

This comment has been minimized.

@myronmarston

myronmarston Jan 7, 2014
Member

Given that the describe doc string is expect(...).to be_between, it seems odd/wrong to use the negated form here. Instead, you can do this:

it 'is exlcusive' do
  expect { expect(10).to be_between(1, 10) }.to fail
  expect { expect(1).to be_between(1, 10) }.to fail
end

(Again, showing both edges is important: otherwise, the implementation could actually be using >= for the low end, as there is no other spec enforcing the fact that it uses >).

This comment has been minimized.

@pedrogimenez

pedrogimenez Jan 7, 2014
Author Contributor

At first, it was that way but I thought it was too verbose then I've changed it. Anyway, I'll change it :)

it_behaves_like "composing with other matchers", :exclusive do
def matcher(min, max)
a_value_between(min, max).exclusive
end

This comment has been minimized.

@myronmarston

myronmarston Jan 7, 2014
Member

I'd like to see all of these example groups scoped inside a describe BeBetween group. Besides the shared example group scoping, it'll improve the doc output -- as this stands, there's a top level example group called composing with other matchers (inclusive) -- which doesn't make much sense on its own, but given we run example groups in random order it'll show up on its own somewhere.

@myronmarston
Copy link
Member

@myronmarston myronmarston commented Jan 7, 2014

@pedrogimenez -- this is looking great. Left a few more comments, but this is almost ready to merge.

@pedrogimenez
Copy link
Contributor Author

@pedrogimenez pedrogimenez commented Jan 7, 2014

Hey @myronmarston, have a look at the code!

end

it 'works with other Comparable objects' do
module RSpec::Matchers::BuiltIn

This comment has been minimized.

@pedrogimenez

pedrogimenez Jan 7, 2014
Author Contributor

I don't like this style but I don't like having three indentation levels either.

JonRowe added a commit that referenced this pull request Jan 8, 2014
Implement exclusive mode in be_between
@JonRowe JonRowe merged commit fc697d0 into rspec:master Jan 8, 2014
1 check passed
1 check passed
default The Travis CI build passed
Details
@JonRowe
Copy link
Member

@JonRowe JonRowe commented Jan 8, 2014

Thanks for the work on this!

JonRowe added a commit that referenced this pull request Jan 8, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.