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

Require VoidExpect to operate inside an example block #1975

Conversation

corsonknowles
Copy link
Contributor

@corsonknowles corsonknowles commented Oct 14, 2024

This PR completes branch coverage for VoidExpect.

This PR copies the inside_example? method and re-uses it here to get more coherent behavior from the VoidExpect cop.

Note that if we were thinking of copying inside_example? a 3rd time, I'd suggest factoring it out into a utility (Sandi Metz' rule of 3).

Note that I remove the return true unless parent guard clause, because we expect the inside_example? guard clause to fully cover that condition.

Split from:


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.
  • Updated documentation.
  • Added an entry to the CHANGELOG.md 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 is configured as Enabled: pending in config/default.yml.
  • The cop is configured as Enabled: true in .rubocop.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.
  • Set VersionAdded: "<<next>>" in default/config.yml.

If you have modified an existing cop's configuration options:

  • Set VersionChanged: "<<next>>" in config/default.yml.

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Thanks!

@rubocop/rubocop-rspec I’d merge this right away, as this allows to remove some nonsensical examples for this cop in the coverage tightening PR.

@corsonknowles corsonknowles force-pushed the require_void_expect_acts_inside_an_example_block branch from 38a6f78 to 3f38ff9 Compare October 14, 2024 20:12
@corsonknowles
Copy link
Contributor Author

@pirj Can you review 1 more time please?

I think I solved this, but it entails removing this guard clause
return true unless parent
in favor of what I believe is the much more restrictive one:
return unless inside_example?(node)

At this point, I can probably drop all VoidExpect changes from the other PR, so it's just amending that 1 other spec file.

@corsonknowles corsonknowles marked this pull request as ready for review October 14, 2024 20:15
@corsonknowles corsonknowles requested a review from a team as a code owner October 14, 2024 20:15
RUBY
end

context 'when expect has no parent node' do
Copy link
Member

Choose a reason for hiding this comment

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

I’d just removed all this except the first one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need the block version below to cover the new guard clause in on_block

I'll delete the rest and get this merged in.

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

A huge 👍 from me, thank you!

@corsonknowles corsonknowles force-pushed the require_void_expect_acts_inside_an_example_block branch from 3f38ff9 to 7484a9a Compare October 14, 2024 21:44
@corsonknowles
Copy link
Contributor Author

@pirj Feel free to merge at will
(or reapprove and I will merge? however this works)

@pirj
Copy link
Member

pirj commented Oct 14, 2024

We usually get consensus on things that might be user-facing, so I’ll pass on merging to the next reviewer.

May I kindly ask to add a short changelog note that the cop was fixed to only inspect code inside examples?

@corsonknowles corsonknowles force-pushed the require_void_expect_acts_inside_an_example_block branch from 7484a9a to 7718880 Compare October 14, 2024 23:05
@corsonknowles
Copy link
Contributor Author

Thanks! CHANGELOG entry added.

@corsonknowles corsonknowles changed the title Require VoidExpect operate inside an example block Require VoidExpect to operate inside an example block Oct 15, 2024
@corsonknowles corsonknowles force-pushed the require_void_expect_acts_inside_an_example_block branch from 7718880 to 3e0cb99 Compare October 15, 2024 11:13
@corsonknowles corsonknowles force-pushed the require_void_expect_acts_inside_an_example_block branch from 3e0cb99 to 7e3149d Compare October 18, 2024 13:15
@corsonknowles
Copy link
Contributor Author

@bquorning Would you be up for doing the 2nd review on this one?

Copy link
Member

@ydah ydah left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Collaborator

@bquorning bquorning left a comment

Choose a reason for hiding this comment

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

Yep, looks good 👍🏼

@bquorning bquorning merged commit 530af44 into rubocop:master Oct 25, 2024
24 checks passed
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.

4 participants