Change AssertEmptyLiteral cop to check for assert_equal
#118
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cstyles
force-pushed
the
fix-assert-empty-literal
branch
from
a78b495
to
b6d465b
Compare
|
cc @tejasbubane who authored this in #38 |
|
This change will be complemented by #120 and will be acceptable. |
koic
reviewed
Feb 21, 2021
CHANGELOG.md
Outdated
| @@ -2,6 +2,10 @@ | |||
|
|
|||
| ## master (unreleased) | |||
|
|
|||
| ### Bug fixes | |||
|
|
|||
| * [#118](https://github.com/rubocop-hq/rubocop-minitest/pull/118): Fix `Minitest/AssertEmptyLiteral` by making it check for `assert_equal([], array)` instead of `assert([], array)`. ([@cstyles][]) | |||
There was a problem hiding this comment.
This is a breaking change. Can you mark **(Breaking)** and squash your commits into one?
Suggested change
| * [#118](https://github.com/rubocop-hq/rubocop-minitest/pull/118): Fix `Minitest/AssertEmptyLiteral` by making it check for `assert_equal([], array)` instead of `assert([], array)`. ([@cstyles][]) | |
| * [#118](https://github.com/rubocop-hq/rubocop-minitest/pull/118): **(Breaking)** Fix `Minitest/AssertEmptyLiteral` by making it check for `assert_equal([], array)` instead of `assert([], array)`. ([@cstyles][]) |
tejasbubane
approved these changes
Feb 21, 2021
While this cop is correct that we should be using `assert_empty(list)` over `assert([], list)`, it's right for the wrong reasons. That example is bad because it's using `assert` and not `assert_equal`. An empty list/hash is truthy so the assertion will always succeed. That is, `assert([], [1, 2, 3])` will succeed even though at a glance it looks like it shouldn't. I believe that what this cop is intending to check for is `assert_equal([], list)` instead so I've updated the references to `assert` to `assert_equal`. Issue rubocop#117 proposes that we try to detect when `assert` is used where the user likely wanted `assert_equal` so while this cop will no longer register a violation for `assert([], list)`, if that proposal is accepted then a future cop *will* register a violation.
cstyles
force-pushed
the
fix-assert-empty-literal
branch
from
b6d465b
to
70f02e0
Compare
|
Nicely done! |
koic
added a commit
that referenced
this pull request
Feb 21, 2021
8 tasks
koic
added a commit
to koic/rubocop-minitest
that referenced
this pull request
Oct 2, 2021
This PR marks `Minitest/AssertEmptyLiteral` as safe auto-correction. Introduced as unsafe auto-correction in rubocop#85, but now safe auto-correction with changes in rubocop#118.
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While this cop is correct that we should be using
assert_empty(list)over
assert([], list), it's right for the wrong reasons. That exampleis bad because it's using
assertand notassert_equal. An emptylist/hash is truthy so the assertion will always succeed. That is,
assert([], [1, 2, 3])will succeed even though at a glance it lookslike it shouldn't. I believe that what this cop is intending to check
for is
assert_equal([], list)instead so I've updated the referencesto
asserttoassert_equal.Issue #117 proposes that we try to detect when
assertis used wherethe user likely wanted
assert_equalso while this cop will no longerregister a violation for
assert([], list), if that proposal isaccepted then a future cop will register a violation.
edit: #120 adds such a cop.
Before submitting the PR make sure the following are checked:
[Fix #issue-number](if the related issue exists).master(if not - rebase it).and description in grammatically correct, complete sentences.
bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.