Rewrite custom matcher DSL #332

myronmarston · 2013-10-04T07:07:29Z

Rewrite custom matcher DSL.

Rather than evaling the define block in the
context of the matcher instance, eval the define
block in the context of the matcher instance's
singleton class.

Fixes Bugfix: makes possible to use RSpec::Matchers#include inside a custom matcher definition #272.
include in define has a different meaning (module inclusion)
than include in the match block (using the include matcher to
match).
Better solution than Allow custom matchers to use built-in #match #194
for match matcher not invoked correctly from custom matchers #188. There's now
a match class method and a match instance method.
Completely avoids issues we had to use hacks to solve before:
Define method in matcher dsl #29,
Fix chain bug #38,
fc4b66d

This also adds a few misc improvements:

Each custom matcher gets its own class, which is assigned to a constant in the RSpec::Matchers::Custom namespace.
I split the large number of methods up into a few modules for greater clarity about what is what: compare Macros vs DefaultImplementations.
All user overrides created by the DSL methods now compile to an actual method, so that users can use early returns, such as:

RSpec::Matchers.define :foo_bar do |expected|
  match do |actual|
    return false if actual == :unexpected
    actual.compares_to?(expected)
  end
end

There were some places where things were underspecified; I added specs for them.
On 1.9+, respond_to_missing? is defined so that matcher.method(:some_example_method) works.
Improved the docs a bit.
chain blocks can now accept blocks.

There is one breaking change that we'll probably want to put warnings in 2.99 about. In a case like this:

RSpec::Matchers.define :foo_bar do |expected|
  extend HelperModuleThatDefinesFoo
  def self.bar; end

  match do |actual|
    bar == foo
  end
end

...it would work previously, because self in the define block was the same as self within match (and the other DSL blocks). Now, self in the define block is the singleton class, so it has the feel of defining a class, with the blocks having instance-level scope. That means that methods from extended modules or defined using self. syntax cannot be called from match as they could before.

I think this is 100% compatible with what we had before otherwise.

Feedback wanted!

/cc @dchelimsky @xaviershay @JonRowe

`do` and `end`, as English words, read funny in the middle of the expectation expression.

- Use a helper method to create the matchers, rather than `Matcher.new` + `for_expected`. I plan to rewrite the implementation of `Matcher` in a way that will require it to be instantiated differently and I having this helper method gives us only one place to update. - Prefer `let` to `before(:each)` ivars. - Define the module inline as a local; this avoids the need for the awkward `m = mod`.

This will help with my upcoming refactoring.

In RSpec 3 the body of `define` will be a class definition and the body of `match` (etc) will be a method def, so using `extend` to make a method available to the `match` block will not work.

Rather than evaling the `define` block in the context of the matcher instance, eval the `define` block in the context of the matcher instance's singleton class. * Fixes #272. `include` in `define` has a different meaning (module inclusion) than `include` in the `match` block (using the `include` matcher to match). * Better solution than #194 for #188. There's now a `match` class method and a `match` instance method. * Completely avoids issues we had to use hacks to solve before: #29, #38, fc4b66d

This helps provides useful `#inspect` output.

- This makes the "this macro method defines a method" abstraction less leaky. - It allows constructs like an early `return` to be used from within the block. - It puts all user refs into a common module.

JRuby and Rubinius apparently don't handle non-ascii in const names.

coveralls · 2013-10-04T07:09:53Z

Coverage decreased (-0.27%) when pulling ec61662 on refactor_matcher_dsl into c7e8424 on master.

[ci skip]

myronmarston · 2013-10-04T07:18:11Z

One other thing...in d3e5310 for_expected was added that attempted to re-use matcher objects. I removed this; it didn't fit as well with the new class/instance split. Plus, it has been the source of multiple bugs (see #112 and fc4b66d) and had sprouted some really hairy logic to ensure stuff wasn't shared that shouldn't have been.

@dchelimsky -- do you remember the original motivation for adding for_expected? Was it a noticable perf improvement? I'm trying to make sure there's nothing important lost here.

xaviershay · 2013-10-04T14:45:35Z

lib/rspec/matchers/matcher.rb

+        # Provides default implementations for the matcher protocol methods.
+        include DefaultImplementations
+
+        # So that expectation expressions can be used in the match block...


I'm not sure what the ellipsis signifies.

Authorial whimsy perhaps.

Something like that. It's a habit I have, for some reason...

Future generations of lit-majors scouring rspec searching for meaning in The Word Of Myron.

xaviershay · 2013-10-04T14:51:05Z

I don't have any substantive feedback, this seems like a good change to me.

They claim you can't use `match` matcher within` match` block, but this now works.

coveralls · 2013-10-04T15:15:48Z

Coverage increased (+0.12%) when pulling 805fe7c on refactor_matcher_dsl into c7e8424 on master.

JonRowe · 2013-10-05T02:43:59Z

lib/rspec/matchers.rb

    def match(expected)
      BuiltIn::Match.new(expected)
    end
-
    alias_method :match_regex, :match


As you've removed the note is this just being included for backwards compatibility now?

Yeah, I considered removing it but there's no maintenance cost in keeping it (it's one line) so given how many other things we're changing in RSpec 3 I didn't see a reason to remove this as well.

Here's what I think I'll do, though....I'll document it as deprecated/"not recommended for use", and we can consider removing it in 4.0.

Perhaps it's worth issuing a deprecation from it...

JonRowe · 2013-10-05T10:54:26Z

Great work! Have only a few minor comments above!

Actually, it probably doesn't matter much here, since we are doing other things there that are likely to blow away the method cache, but we might as well avoid one more runtime extension.

Rewrite custom matcher DSL

myronmarston · 2013-10-09T07:30:53Z

I went ahead and merged, now that I have #337 open for the 2.99 deprecations.

myronmarston · 2013-10-09T07:31:37Z

Oh yeah, I wanted to benchmark matchers built using this compared to 2.14, and see if there is a change, either way. I may still do that.

dchelimsky · 2013-10-14T11:50:27Z

I think that benchmark is very important, and recommend it is added before a release.

myronmarston · 2013-10-14T15:51:09Z

I think that benchmark is very important, and recommend it is added before a release.

Yep, I opened #338 to not forget that work. I plan to work on that next.

dchelimsky · 2013-10-14T15:53:07Z

Thx!

myronmarston added 15 commits October 2, 2013 21:38

Prefer curlies to do-end for expect { }.to raise_error.

8300bf8

`do` and `end`, as English words, read funny in the middle of the expectation expression.

Address a few under specifications of custom matchers.

eaf1c14

This will help with my upcoming refactoring.

Use include instead of extend.

0ed7323

In RSpec 3 the body of `define` will be a class definition and the body of `match` (etc) will be a method def, so using `extend` to make a method available to the `match` block will not work.

Add pending specs for things that should work after matcher refactoring.

fa3e980

Ensure helper methods can be accessed from the macro blocks.

6617bfc

Use singleton classes rather than dup'ing the matcher class.

1d5c7f4

Assign the matcher subclass to a human readable const.

28d715e

This helps provides useful `#inspect` output.

Compile the user's block directly to a method.

f5cd114

- This makes the "this macro method defines a method" abstraction less leaky. - It allows constructs like an early `return` to be used from within the block. - It puts all user refs into a common module.

Allow chain blocks to accept blocks.

c05616e

Use respond_to_missing? on 1.9+.

63cdacc

Improve docs.

e57fc6d

Make matcher constant assignment more robust.

e9490b0

JRuby and Rubinius apparently don't handle non-ascii in const names.

for_expected doesn't really serve a purpose here.

ec61662

Add changelog for recent custom matcher changes.

9e05e56

[ci skip]

myronmarston mentioned this pull request Oct 4, 2013

Bugfix: makes possible to use RSpec::Matchers#include inside a custom matcher definition #272

Closed

xaviershay reviewed Oct 4, 2013
View reviewed changes

Remove misleading docs.

805fe7c

They claim you can't use `match` matcher within` match` block, but this now works.

JonRowe reviewed Oct 5, 2013
View reviewed changes

myronmarston added 2 commits October 6, 2013 18:04

No need for the ellipses.

44c9d56

We're trying to get away from run-time extensions.

1a5e7d5

Actually, it probably doesn't matter much here, since we are doing other things there that are likely to blow away the method cache, but we might as well avoid one more runtime extension.

myronmarston added 3 commits October 6, 2013 18:07

Add explanatory comment.

1fa9651

It's "capital", not "capitol".

08eda0c

Add deprecation notice to API docs for match_regex.

9239370

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

Merge pull request #332 from rspec/refactor_matcher_dsl

42df8d3

Rewrite custom matcher DSL

myronmarston merged commit 42df8d3 into master Oct 9, 2013

myronmarston deleted the refactor_matcher_dsl branch October 9, 2013 07:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite custom matcher DSL #332

Rewrite custom matcher DSL #332

myronmarston commented Oct 4, 2013

coveralls commented Oct 4, 2013

myronmarston commented Oct 4, 2013

xaviershay Oct 4, 2013

JonRowe Oct 5, 2013

myronmarston Oct 5, 2013

xaviershay Oct 5, 2013

xaviershay commented Oct 4, 2013

coveralls commented Oct 4, 2013

JonRowe Oct 5, 2013

myronmarston Oct 5, 2013

JonRowe Oct 5, 2013

JonRowe commented Oct 5, 2013

myronmarston commented Oct 9, 2013

myronmarston commented Oct 9, 2013

dchelimsky commented Oct 14, 2013

myronmarston commented Oct 14, 2013

dchelimsky commented Oct 14, 2013

Rewrite custom matcher DSL #332

Rewrite custom matcher DSL #332

Conversation

myronmarston commented Oct 4, 2013

coveralls commented Oct 4, 2013

myronmarston commented Oct 4, 2013

xaviershay Oct 4, 2013

Choose a reason for hiding this comment

JonRowe Oct 5, 2013

Choose a reason for hiding this comment

myronmarston Oct 5, 2013

Choose a reason for hiding this comment

xaviershay Oct 5, 2013

Choose a reason for hiding this comment

xaviershay commented Oct 4, 2013

coveralls commented Oct 4, 2013

JonRowe Oct 5, 2013

Choose a reason for hiding this comment

myronmarston Oct 5, 2013

Choose a reason for hiding this comment

JonRowe Oct 5, 2013

Choose a reason for hiding this comment

JonRowe commented Oct 5, 2013

myronmarston commented Oct 9, 2013

myronmarston commented Oct 9, 2013

dchelimsky commented Oct 14, 2013

myronmarston commented Oct 14, 2013

dchelimsky commented Oct 14, 2013