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

feat: support correcting assert_nil and refute_nil to RSpec/Rails/MinitestAssertions #1773

Merged
merged 1 commit into from
Jan 6, 2024

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented Jan 5, 2024

I'm not sure how far folks want to take this (if at all), but I got the impression from rubocop/rubocop-rspec_rails#7 & #1517 that it was desirable to support at least some more assertions, and this felt like a pretty straightforward one to land.

I've deliberately gone the "boring" route of just effectively duplicating everything twice rather than try to use a single set of methods because lines of code are cheap and if more matchers are supported there might be better ways to deduplicate that are not compatible with one or both of these matchers.


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.

@G-Rath G-Rath requested a review from a team as a code owner January 5, 2024 19:10
@G-Rath
Copy link
Contributor Author

G-Rath commented Jan 5, 2024

Initially I thought it would be best to correct to be_nil since that's what other cops already do for to eq(nil) but I realised that's why it actually shouldn't because Rubocop will automatically apply that fix on the same pass and then people who have turned off the default cops for eq(nil) -> be(nil) -> be_nil will end up with their preferred syntax too.

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.

Looks good!

Perhaps consider wrapping specs in two context blocks, one for the “equal” assertions and one for the “nil” assertions.

@G-Rath
Copy link
Contributor Author

G-Rath commented Jan 5, 2024

oh yeah good point! I've been trailing minitest recently and so had completely forgotten we've got context here 🙈

@bquorning
Copy link
Collaborator

@pirj, could I get a second opinion on this PR, please?

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.

Fantastic, thank you!

context 'with equal assertions' do
it 'registers an offense when using `assert_equal`' do
expect_offense(<<~RUBY)
assert_equal(a, b)
Copy link
Member

Choose a reason for hiding this comment

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

It was a miss in the original implementation, it’s confusing that a is “expected, and b is “actual”.
Up to you if you want to fix this along the way or leave out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point - I'll leave it for now since this diff is already pretty big, but happy to do a follow up addressing it

@pirj pirj merged commit bd8e3b9 into rubocop:master Jan 6, 2024
24 checks passed
@pirj
Copy link
Member

pirj commented Jan 6, 2024

Thanks again!

@@ -1169,6 +1169,7 @@ RSpec/Rails/MinitestAssertions:
Description: Check if using Minitest matchers.
Enabled: pending
VersionAdded: '2.17'
VersionChanged: "<<next>>"
Copy link
Member

Choose a reason for hiding this comment

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

This change should not have been made. This should only be changed when the default settings are changed. For example, when you add a configurable option or change the default for a configurable option.

Copy link
Member

Choose a reason for hiding this comment

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

fix: #1777

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops, my bad. I should have caught that in review.

Thanks for discovering, and fixing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for what it's worth I feel like that was required by bundle exec rake, but it might have been I just messed something up or changed something else I shouldn't have locally to make it think this needed updating 🤷

I'll keep this in mind for future contributions and let you know if I do find the tasks behaving wrongly

Copy link
Member

Choose a reason for hiding this comment

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

Never mind! Thank you for your work.

ydah added a commit that referenced this pull request Jan 10, 2024
@ydah ydah mentioned this pull request Jan 10, 2024
5 tasks
ydah added a commit to rubocop/rubocop-rspec_rails that referenced this pull request Mar 27, 2024
ydah added a commit to rubocop/rubocop-rspec_rails that referenced this pull request Mar 27, 2024
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