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

Conversation

@pirj
Copy link
Member

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-hq/rspec-style-guide#76
rubocop-hq/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

This comment has been minimized.

Copy link
Member Author

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?

@Darhazer

This comment has been minimized.

Copy link
Member

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

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

@pirj

This comment has been minimized.

Copy link
Member Author

commented Jul 24, 2019

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

@Darhazer
Copy link
Member

left a comment

Let fist merge the style guide though

@pirj

This comment has been minimized.

Copy link
Member Author

commented Jul 24, 2019

Let fist merge the style guide though

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

@Darhazer

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

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

@pirj

This comment has been minimized.

Copy link
Member Author

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.

@pirj
Copy link
Member Author

left a comment

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

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

This comment has been minimized.

Copy link
@pirj

pirj Jul 24, 2019

Author Member

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)).


def on_send(node)
implicit_expect(node) do |implicit_expect|
subject = nearest_subject(implicit_expect)

This comment has been minimized.

Copy link
@pirj

pirj Jul 24, 2019

Author Member

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

def nearest_subject(node)
node
.each_ancestor(:block)
.lazy

This comment has been minimized.

Copy link
@pirj

pirj Jul 24, 2019

Author Member

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

end

def multi_statement_example_group?(node)
example_group_with_body?(node) && node.body.begin_type?

This comment has been minimized.

Copy link
@pirj

pirj Jul 24, 2019

Author Member

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.

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

def_node_matcher :implicit_expect, <<-PATTERN

This comment has been minimized.

Copy link
@pirj

pirj Jul 24, 2019

Author Member

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

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

@Darhazer
Copy link
Member

left a comment

Excellent work. Few minor comments

lib/rubocop/cop/rspec/implicit_block_expectation.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/implicit_block_expectation.rb Outdated Show resolved Hide resolved

@pirj pirj force-pushed the pirj:add-implicit-block-expectation-cop branch 3 times, most recently from 0c8450f to 7f135df Jul 25, 2019

@pirj pirj force-pushed the pirj: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)

This comment has been minimized.

Copy link
@bquorning

bquorning Jul 31, 2019

Member

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

This comment has been minimized.

Copy link
@pirj

pirj Jul 31, 2019

Author Member

Yes, I think those are equivalent.

@bquorning bquorning merged commit 0d8c1f3 into rubocop-hq:master Jul 31, 2019

11 checks passed

ci/circleci: code-climate Your tests passed on CircleCI!
Details
ci/circleci: confirm_config_and_documentation Your tests passed on CircleCI!
Details
ci/circleci: jruby Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.3-rspec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.3-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.4-rspec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.4-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.5-rspec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.5-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.6-rspec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.6-rubocop Your tests passed on CircleCI!
Details

@pirj pirj deleted the pirj:add-implicit-block-expectation-cop branch Jul 31, 2019

@pirj

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2019

Thank you!

@bquorning

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

And thank you for the new cop :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.