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 custom RuboCop for `assert_not` over `refute` #32441

Merged
merged 2 commits into from Apr 4, 2018

Conversation

Projects
None yet
7 participants
@composerinteralia
Member

composerinteralia commented Apr 4, 2018

Since at least cf4afc4 we have preferred assert_not methods over refute
methods in our tests. I have seen plenty of comments in PRs about this, and we have
tried to fix it a few times (5294ad8, e45f176, 8910f12, 41f50be,
d4cfd54, 48a183e, and 211adb4), but the refute methods keep sneaking back in.

This custom RuboCop will take care of enforcing this preference, so we
don't have to think about it again. I suspect there are other similar
stylistic preferences that could be solved with some custom RuboCops, so
I will definitely keep my eyes open. assert_not over assert ! might
be a good candidate, for example.

I wasn't totally sure if ci/custom_cops was the best place to put
this, but nothing else seemed quite right. At one point I had it set up
as a gem, but I think custom cops like this would have limited value
in another context.

I want to see how code climate handles the new cops before
autocorrecting the existing violations. If things go as expected, I will
push another commit with those corrections.

Failed the build as expected. rubocop -a fixed all the new violations, so hopefully code climate is happy this time.

@rails-bot

This comment has been minimized.

rails-bot commented Apr 4, 2018

r? @kaspth

(@rails-bot has picked a reviewer for you, use r? to override)

@sikachu

This comment has been minimized.

Member

sikachu commented Apr 4, 2018

we have preferred refute methods over assert_not
methods in our tests.

Did you actually meant to write "we have preferred assert_not over refute"?

@composerinteralia

This comment has been minimized.

Member

composerinteralia commented Apr 4, 2018

Yes! ACK!

composerinteralia added some commits Apr 4, 2018

Add custom RuboCop for `assert_not` over `refute`
Since at least cf4afc4 we have preferred `assert_not` methods over
`refute` methods. I have seen plenty of comments in PRs about this,
and we have tried to fix it a few times (5294ad8, e45f176, 8910f12,
41f50be, d4cfd54, 48a183e, and 211adb4), but the `refute` methods
keep sneaking back in.

This custom RuboCop will take care of enforcing this preference, so we
don't have to think about it again. I suspect there are other similar
stylistic preferences that could be solved with some custom RuboCops, so
I will definitely keep my eyes open. `assert_not` over `assert !` might
be a good candidate, for example.

I wasn't totally sure if `ci/custom_cops` was the best place to put
this, but nothing else seemed quite right. At one point I had it set up
as a gem, but I think custom cops like this would have limited value
in another context.

I want to see how code climate handles the new cops before
autocorrecting the existing violations. If things go as expected, I will
push another commit with those corrections.
Autocorrect `refute` RuboCop violations
73e7aab behaved as expected on codeship, failing the build with
exactly these RuboCop violations. Hopefully `rubocop -a` will
have been enough to get a passing build!
@matthewd

This comment has been minimized.

Member

matthewd commented Apr 4, 2018

cc @toshimaru

@rafaelfranca rafaelfranca merged commit fe4e9d4 into rails:master Apr 4, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@composerinteralia composerinteralia deleted the composerinteralia:refute-not branch Apr 7, 2018

koic added a commit to koic/rubocop-rails-style that referenced this pull request Apr 8, 2018

Backport new `CustomCops/RefuteNotTest` cop from rails/rails
This PR backports rails/rails#32441.

IMO, I think that it would be more plausible to write
`require: rubocop-rails` instead of `require: custom_cops`
in config/rails.yml, but first of all this PR writes
`require: custom_cops` according to upstream.

koic added a commit to koic/rubocop-rails-style that referenced this pull request Apr 8, 2018

Import new `CustomCops/RefuteNotTest` cop from rails/rails
This PR imports rails/rails#32441.

IMO, I think that it would be more plausible to write
`require: rubocop-rails` instead of `require: custom_cops`
in config/rails.yml, but first of all this PR writes
`require: custom_cops` according to upstream.
@koic

This comment has been minimized.

Contributor

koic commented Apr 19, 2018

I think this cop has a useful opportunity in Rails applications. Therefore I will export this cop and open PR to RuboCop. It will also reduce dependency from Rails.

koic added a commit to koic/rubocop that referenced this pull request Apr 19, 2018

Add new `Rails/RefuteMethods` cop
## Summary

This PR imports a custom cop from rails/rails#32441.
The cop name has changed, but behavior is the same.

This cop checks for use of `refute` method series.

```ruby
# bad
refute false
refute_empty [1, 2, 3]
refute_equal true, false

# good
assert_not false
assert_not_empty [1, 2, 3]
assert_not_equal true, false
```

`assert_not` methods are not minitest's memthods.
It is an aliases defined in Active Support (`ActiveSupport::TestCase`).
https://github.com/rails/rails/blob/v5.2.0/activesupport/lib/active_support/test_case.rb#L54-L68

So I put this cop in Rails department.

I think this cop has a useful opportunity in Rails applications.

## Other Information

In the near future this PR aims to replace `CustomCops/RefuteNot` cop of
rails/rails repo with this `Rails/RefuseMethods` cop.

@koic koic referenced this pull request Apr 19, 2018

Merged

Add new `Rails/RefuteMethods` cop #5801

7 of 8 tasks complete

composerinteralia added a commit to composerinteralia/rails that referenced this pull request Apr 19, 2018

Add RuboCop for `assert_not` over `assert !`
We added `assert_not` in f75addd "to replace warty 'assert !foo'".
fa8d35b agrees that it is warty, and so do I. This custom Rubocop rule
turns the wart into a violation.

As with my last custom cop, rails#32441,
I want to make sure this looks right on code climate before pushing
another commit to autocorrect everything.

@toshimaru I just noticed
toshimaru/rubocop-rails#26
Is there a better way to add these custom cops, or were you saying we
shouldn't have custom cops at all?

koic added a commit to koic/rubocop that referenced this pull request Apr 19, 2018

Add new `Rails/RefuteMethods` cop
## Summary

This PR imports a custom cop from rails/rails#32441.
The cop name has changed, but behavior is the same.

This cop checks for use of `refute` method series.

```ruby
# bad
refute false
refute_empty [1, 2, 3]
refute_equal true, false

# good
assert_not false
assert_not_empty [1, 2, 3]
assert_not_equal true, false
```

`assert_not` methods are not minitest's memthods.
It is an aliases defined in Active Support (`ActiveSupport::TestCase`).
https://github.com/rails/rails/blob/v5.2.0/activesupport/lib/active_support/test_case.rb#L54-L68

So I put this cop in Rails department.

I think this cop has a useful opportunity in Rails applications.

## Other Information

In the near future this PR aims to replace `CustomCops/RefuteNot` cop of
rails/rails repo with this `Rails/RefuseMethods` cop.

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Apr 19, 2018

Allow rubocop check more files
This commit fix pattern of filenames for `CustomCops/AssertNot` and
`CustomCops/RefuteNot`.

rubocop should check every file under `test/`.

Related to rails#32441, rails#32605

koic added a commit to koic/rubocop that referenced this pull request Apr 20, 2018

Add new `Rails/RefuteMethods` cop
## Summary

This PR imports a custom cop from rails/rails#32441.
The cop name has changed, but behavior is the same.

This cop checks for use of `refute` method series.

```ruby
# bad
refute false
refute_empty [1, 2, 3]
refute_equal true, false

# good
assert_not false
assert_not_empty [1, 2, 3]
assert_not_equal true, false
```

`assert_not` methods are not minitest's memthods.
It is an aliases defined in Active Support (`ActiveSupport::TestCase`).
https://github.com/rails/rails/blob/v5.2.0/activesupport/lib/active_support/test_case.rb#L54-L68

So I put this cop in Rails department.

I think this cop has a useful opportunity in Rails applications.

## Other Information

In the near future this PR aims to replace `CustomCops/RefuteNot` cop of
rails/rails repo with this `Rails/RefuseMethods` cop.

bbatsov added a commit to rubocop-hq/rubocop that referenced this pull request Apr 21, 2018

Add new `Rails/RefuteMethods` cop
## Summary

This PR imports a custom cop from rails/rails#32441.
The cop name has changed, but behavior is the same.

This cop checks for use of `refute` method series.

```ruby
# bad
refute false
refute_empty [1, 2, 3]
refute_equal true, false

# good
assert_not false
assert_not_empty [1, 2, 3]
assert_not_equal true, false
```

`assert_not` methods are not minitest's memthods.
It is an aliases defined in Active Support (`ActiveSupport::TestCase`).
https://github.com/rails/rails/blob/v5.2.0/activesupport/lib/active_support/test_case.rb#L54-L68

So I put this cop in Rails department.

I think this cop has a useful opportunity in Rails applications.

## Other Information

In the near future this PR aims to replace `CustomCops/RefuteNot` cop of
rails/rails repo with this `Rails/RefuseMethods` cop.

koic added a commit to rubocop-hq/rubocop-rails that referenced this pull request Oct 26, 2018

Add new `Rails/RefuteMethods` cop
## Summary

This PR imports a custom cop from rails/rails#32441.
The cop name has changed, but behavior is the same.

This cop checks for use of `refute` method series.

```ruby
# bad
refute false
refute_empty [1, 2, 3]
refute_equal true, false

# good
assert_not false
assert_not_empty [1, 2, 3]
assert_not_equal true, false
```

`assert_not` methods are not minitest's memthods.
It is an aliases defined in Active Support (`ActiveSupport::TestCase`).
https://github.com/rails/rails/blob/v5.2.0/activesupport/lib/active_support/test_case.rb#L54-L68

So I put this cop in Rails department.

I think this cop has a useful opportunity in Rails applications.

## Other Information

In the near future this PR aims to replace `CustomCops/RefuteNot` cop of
rails/rails repo with this `Rails/RefuseMethods` cop.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment