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

Add ImplicitBlockExpectation cop #789

Merged
merged 1 commit into from Jul 31, 2019

Conversation

@pirj
Copy link
Member

@pirj pirj commented Jul 23, 2019

Based on https://rspec.rubystyle.guide/#implicit-block-expectations

The implicit syntax is discouraged to use by RSpec Core team and the majority of voters.

There were no good arguments for using the syntax except for brevity and avoiding repetition, but there are better options to achieve the same goal, e.g. by extracting the lengthy block to methods (instead of putting it inside the lambda).

It's barely possible to restrict the usage of this syntax on RSpec side. It's possible to detect if a matcher supports block expectations, but it's impossible to detect if it exclusively supports block expectations and not value expectations. A matcher may support both styles at the same time.
An attempt that results in 86 spec failures pirj/rspec-expectations@a7f0d7a

References:
https://blog.rubystyle.guide/rspec/2019/07/17/rspec-implicit-block-syntax.html
https://www.reddit.com/r/ruby/comments/cejl3q/call_for_discussion_rspec_implicit_block/
https://lobste.rs/s/e8yxmd/call_for_discussion_rspec_implicit_block
rubocop/rspec-style-guide#76
rubocop/rspec-style-guide#103


Before submitting the PR make sure the following are checked:

  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the changelog if the new code introduces user-observable changes.
  • The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

If you have created a new cop:

  • Added the new cop to config/default.yml.
  • The cop documents examples of good and bad code.
  • The tests assert both that bad code is reported and that good code is not reported.
    Tested on a 10k+ files repo, no false offences raised.
@pirj
Copy link
Member Author

@pirj pirj commented Jul 24, 2019

It could be written with the use of def_node_search:

        def_node_matcher :lambda?, <<-PATTERN
          {
            (send (const nil? :Proc) :new)
            (send nil? :proc)
            (send nil? :lambda)
          }
        PATTERN
        def_node_matcher :lambda_block, '$(block #lambda? ...)'
        def_node_search :subject_block, Subject::ALL.block_pattern

        def on_block(node)
          subject_block(node) do |block_node|
            lambda_block(block_node.body) { |lambda| add_offense(lambda) }
          end
        end

I'm wary with def_node_search knowing its recursive nature, but couldn't find a case when it fails.

Which do you prefer, @Darhazer, the current or the one above?

Loading

@Darhazer
Copy link
Contributor

@Darhazer Darhazer commented Jul 24, 2019

def_node_search will perform the recursive search on each block. If you go in this direction, you should limit the search only in the top level describe, to avoid wasting CPU cycles.
I'm ok with the previous version though

Loading

@pirj pirj force-pushed the add-implicit-block-expectation-cop branch from d04e9f9 to 571542f Jul 24, 2019
@pirj
Copy link
Member Author

@pirj pirj commented Jul 24, 2019

Thanks, pushed the former solution without def_node_search and squashed the commits.

Loading

Copy link
Contributor

@Darhazer Darhazer left a comment

Let fist merge the style guide though

Loading

@pirj
Copy link
Member Author

@pirj pirj commented Jul 24, 2019

Let fist merge the style guide though

Agree. 👍
Looking forward to your approval of the style guide pull request.

Loading

@Darhazer
Copy link
Contributor

@Darhazer Darhazer commented Jul 24, 2019

Just noticed that the cop flags subject { -> ... } while the guide only suggests avoiding is_expected with such a subject ;)

Loading

@pirj
Copy link
Member Author

@pirj pirj commented Jul 24, 2019

Fair enough.
I couldn't find any good reason to put a lambda to subject other than to use is_expected, but it's better to be explicit on what is detected, and the offence message might be confusing when attached to subject definition as opposed to is_expected.

I'll take care of that, will add detection of is_expected usages in the scope where subject is defined with a lambda.

Loading

Copy link
Member Author

@pirj pirj left a comment

Added notes to facilitate code review.
@bquorning Can you please take a look?

Loading

describe do
subject { -> { boom } }
it { should change { something }.to(new_value) }
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid implicit block expectations.
Copy link
Member Author

@pirj pirj Jul 24, 2019

Choose a reason for hiding this comment

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

The range is not ideal, but I decided to avoid complicating the cop for the sake of highlighting should itself.
The difference is that is_expected is a self-contained send, while for should it's the whole should(change { something }.to(new_value)).

Loading


def on_send(node)
implicit_expect(node) do |implicit_expect|
subject = nearest_subject(implicit_expect)
Copy link
Member Author

@pirj pirj Jul 24, 2019

Choose a reason for hiding this comment

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

We're finding the nearest subject, and only then checking if it's a lambda subject.
Check this example.

Loading

def nearest_subject(node)
node
.each_ancestor(:block)
.lazy
Copy link
Member Author

@pirj pirj Jul 24, 2019

Choose a reason for hiding this comment

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

lazy is here to avoid mapping/selecting through all of the ancestors just to find the nearest subject.

Loading

end

def multi_statement_example_group?(node)
example_group_with_body?(node) && node.body.begin_type?
Copy link
Member Author

@pirj pirj Jul 24, 2019

Choose a reason for hiding this comment

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

Non-begin_type blocks (example groups) are no interest for us, they don't have other blocks than those we came from, and we're searching for subject. If there are at least two statements in the block, it's a begin_type.

Loading

(block _ _args $(block #lambda? ...))
PATTERN

def_node_matcher :implicit_expect, <<-PATTERN
Copy link
Member Author

@pirj pirj Jul 24, 2019

Choose a reason for hiding this comment

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

Shamelessly borrowed from ImplicitSubject. Not sure if it makes sense to extract this with just two usages.

Loading

@pirj pirj force-pushed the add-implicit-block-expectation-cop branch from 0df2736 to a92f085 Jul 24, 2019
Copy link
Contributor

@Darhazer Darhazer left a comment

Excellent work. Few minor comments

Loading

lib/rubocop/cop/rspec/implicit_block_expectation.rb Outdated Show resolved Hide resolved
Loading
lib/rubocop/cop/rspec/implicit_block_expectation.rb Outdated Show resolved Hide resolved
Loading
@pirj pirj force-pushed the add-implicit-block-expectation-cop branch 3 times, most recently from 0c8450f to 7f135df Jul 30, 2019
@pirj pirj force-pushed the add-implicit-block-expectation-cop branch from 7f135df to 02e41c7 Jul 31, 2019
.lazy
.select { |block_node| multi_statement_example_group?(block_node) }
.map { |block_node| find_subject(block_node) }
.find(&:itself)
Copy link
Collaborator

@bquorning bquorning Jul 31, 2019

Choose a reason for hiding this comment

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

This line is equivalent to .reject(&:nil?).first, right? Just trying to understand.

Loading

Copy link
Member Author

@pirj pirj Jul 31, 2019

Choose a reason for hiding this comment

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

Yes, I think those are equivalent.

Loading

@bquorning bquorning merged commit 0d8c1f3 into rubocop:master Jul 31, 2019
11 checks passed
Loading
@pirj pirj deleted the add-implicit-block-expectation-cop branch Jul 31, 2019
@pirj
Copy link
Member Author

@pirj pirj commented Jul 31, 2019

Thank you!

Loading

@bquorning
Copy link
Collaborator

@bquorning bquorning commented Jul 31, 2019

And thank you for the new cop :-)

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants