Skip to content

Move the have_rendered matcher to RSpec public API. #1968

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

Closed
wants to merge 1 commit into from

Conversation

fables-tales
Copy link
Member

Starting to move RSpec Rails off RSpec's internal API this shows a
sample move of a matcher to pure RSpec public api.

Can I get some eyes @JonRowe @myronmarston ?

Starting to move RSpec Rails off RSpec's internal API this shows a
sample move of a matcher to pure RSpec public api.
class RenderTemplateMatcher < RSpec::Matchers::BuiltIn::BaseMatcher
def initialize(scope, expected, message = nil)
@expected = Symbol === expected ? expected.to_s : expected
class RenderTemplateMatcher < RSpec::Matchers::DSL::Matcher
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you subclassing RSpec::Matchers::DSL::Matcher? Using the matcher DSL is part of our public API, but directly subclassing this class, and dealing with internals such as what args are passed to its initializer, is not part of our public API. Besides that, it forces you to deal with some weird concerns that don't really belong here (such as name: the DSL matcher needs to know what the matcher method name was, but that's static in your case), and define logic using a weird declarations lambda in initialize, which is far harder to follow than just defining the protocol methods. There's also risk of your instance variables conflicting with the matcher DSLs since they live in the same namespace.

Instead, either use the matcher DSL directly, or define a class that doesn't subclass any rspec-expectations class, and fully implements the matcher protocol.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one cannot capture the scope part without directly calling the DSL. My understanding was that inheriting DSL::Matcher was public api. @myronmarston do you have a preferred way to capture the Rails scope given what APIs are available?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one cannot capture the scope part without directly calling the DSL.

That's not true. Consider that this matcher was previously capturing the scope w/o using the DSL at all. And as it happens, RSpec::Matchers::BuiltIn::BaseMatcher doesn't do anything related to the scope.

As far as I can see, the DSL does only 3 things related to the scope:

  1. In the matcher method it defines, it passes self (the scope, as you're calling it) to the constructor when instantiating the matcher. You're doing this part manually in have_rendered and render_template, so you're not even using the DSL for this.
  2. In initialize, the DSL class stores the scope in an instance variable (called @matcher_execution_context). You're doing this part manually do, storing it in a variable called @scope.
  3. The DSL implements respond_to_missing? and method_missing to delegate to the scope so that DSL-defined matchers can call methods defined in the example scope. As far as I can tell, you don't need this, considering the old base class (RSpec::Matchers::BuiltIn::BaseMatcher) did not define this, but if you want it, you can define those methods yourself.

Given you do #1 and #2 yourself already, and it doesn't appear you need #3, I can't see what, if anything, the DSL base class is doing for you with respect to the scope.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use the define_matcher macro to do this without breaking into the class?

@JonRowe
Copy link
Member

JonRowe commented Mar 11, 2020

Closing as stale as this was already done (differently) in the main 4-0 branch.

@JonRowe JonRowe closed this Mar 11, 2020
@pirj pirj deleted the move-have-rendered-to-public-api branch March 12, 2020 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants