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

Include a Cop to enforce assert_nil over assert_equal(nil) #6

Merged
merged 1 commit into from
Aug 31, 2019

Conversation

duduribeiro
Copy link
Contributor

Hey folks 👋

This commit adds a Cop that checks some usage of
assert_equal(nil, something) and enforces the usage of
assert_nil(something) instead.

I don't know if the cop that I wrote is right and maybe it needs some adjustment 😂

I also needed to copy some code from the rubocop gem itself, because the matchers itself relies on RSpec. Maybe we can change it in rubocop gem to share part of the code that can be reusable, and add the matchers for rspec and minitest.

@koic
Copy link
Member

koic commented Aug 16, 2019

I also needed to copy some code from the rubocop gem itself, because the matchers itself relies on RSpec.

Awesome work! It ’s a great first step. We can hack in the future based on the code base. 👍

@koic
Copy link
Member

koic commented Aug 16, 2019

Can you add the setting of this cop to config/default.yml? The following is an example and let's set VersionAdded: '0.1.0'.
https://github.com/rubocop-hq/rubocop-rails/blob/v2.3.0/config/default.yml#L58-L61

test/test_helper.rb Outdated Show resolved Hide resolved
@duduribeiro duduribeiro force-pushed the add_cop_for_assert_nil branch 2 times, most recently from 73ffdab to cf2f301 Compare August 16, 2019 15:02
@duduribeiro
Copy link
Contributor Author

hey @koic 👋

I did update the code with the suggested changes.

I changed the node_matcher pattern to capture the argument: https://github.com/rubocop-hq/rubocop-minitest/pull/6/files#diff-4e8bec16638093f13a99617349303b4bR18

can you check if it is valid? thanks so much!

☕️

@koic
Copy link
Member

koic commented Aug 17, 2019

The def_node_matcher pattern looks good, but false positives occur when writing a message to assert_equal.

assert_equal(nil, actual, 'message')

https://github.com/seattlerb/minitest/blob/ab39d35fb4e84eb866ed9c4ecb707cbf3889de42/lib/minitest/assertions.rb#L192

@duduribeiro duduribeiro force-pushed the add_cop_for_assert_nil branch 2 times, most recently from 8e5349d to e4aed88 Compare August 19, 2019 01:44
@duduribeiro
Copy link
Contributor Author

duduribeiro commented Aug 19, 2019

hey @koic 👋

I updated the code. WDYT with the changes ?

thanks

☕️

config/default.yml Outdated Show resolved Hide resolved
config/default.yml Outdated Show resolved Hide resolved
@duduribeiro duduribeiro force-pushed the add_cop_for_assert_nil branch 2 times, most recently from a602299 to 7e0f733 Compare August 22, 2019 12:56
@duduribeiro
Copy link
Contributor Author

hey @koic, I did apply your changes, can you review? thanks so much

@bbatsov
Copy link
Contributor

bbatsov commented Aug 23, 2019

@duduribeiro I see all tests are failing due to passing the wrong number of arguments to one method.

@duduribeiro duduribeiro force-pushed the add_cop_for_assert_nil branch 2 times, most recently from 4ec0910 to 5e6d9fc Compare August 23, 2019 21:31
@duduribeiro
Copy link
Contributor Author

@bbatsov and @koic I rebased with master and it fixed.. thanks

@duduribeiro
Copy link
Contributor Author

@koic I updated the code

@duduribeiro
Copy link
Contributor Author

hey @koic 👋

I changed the node pattern to allow message to be a variable and a constant and also allowing the method call. Can you check please?

thanks so much ☕️

test/test_helper.rb Outdated Show resolved Hide resolved
@duduribeiro
Copy link
Contributor Author

@koic I changed with your suggestions. Thanks so much

config/default.yml Outdated Show resolved Hide resolved
This commit adds a Cop that checks some usage of
`assert_equal(nil, something)` and enforces the usage of
`assert_nil(something)` instead.
Copy link
Member

@koic koic left a comment

Choose a reason for hiding this comment

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

Many thanks for your great work!

@duduribeiro
Copy link
Contributor Author

@koic thanks for your help!

@koic
Copy link
Member

koic commented Aug 30, 2019

@bbatsov I'd like to release RuboCop Minitest 0.1.0 after merging this PR and maintaining some documentation. This cop is the first cop that will be published on RuboCop Minitest.
This is a great achievement by @duduribeiro.

@bbatsov
Copy link
Contributor

bbatsov commented Aug 30, 2019

@koic I completely agree. Let's make this happen. 😄

@koic koic merged commit 9765f9e into rubocop:master Aug 31, 2019
@duduribeiro duduribeiro deleted the add_cop_for_assert_nil branch October 3, 2019 21:21
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