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

Add new `Style/MixinUsage` cop #4840

Merged
merged 1 commit into from Oct 17, 2017

Conversation

Projects
None yet
5 participants
@koic
Copy link
Member

commented Oct 6, 2017

Feature

Checks that include, extend and prepend exists at the top level.

Using these at the top level affects the behavior of Object. Under the app directory of the Rails application, the convention that a file name and a class name are mapped is applied.

% cat app/models/example.rb
# frozen_string_literal: true

include M

class Example < ApplicationRecord
end
% bundle exec rubocop app/models/example.rb
Inspecting 1 file
C

Offenses:

app/models/example.rb:3:1: C: Rails/TopLevelInclude: include is used at the top level. Use inside class or module.
include M
^^^^^^^^^

1 file inspected, 1 offense detected

Since it is difficult to automatically determine the position of include for class including multiple modules, autocorrect is not provided.


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Used the same coding conventions as the rest of the project.
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • All tests(rake spec) are passing.
  • The new code doesn't generate RuboCop offenses that are checked by rake internal_investigation.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Updated cop documentation with rake generate_cops_documentation (required only when you've added a new cop or changed the configuration/documentation of an existing cop).

@koic koic force-pushed the koic:add_new_rails_top_level_include_cop branch from acd313b to 2b63125 Oct 6, 2017

PATTERN

def on_send(node)
return unless (statement = include_statement(node))

This comment has been minimized.

Copy link
@Drenmi

Drenmi Oct 6, 2017

Collaborator

Hm. Looks like you managed to trick Lint/AssignmentInCondition by using parentheses. 😅 Will open an issue for that.

This comment has been minimized.

Copy link
@Drenmi

Drenmi Oct 6, 2017

Collaborator

Never mind. This is the intended behaviour. Can be disabled by setting AllowSafeAssignment: false. 🙂


def top_level_node?(node)
if node.parent.parent.nil?
node.parent.children.first == node

This comment has been minimized.

Copy link
@Drenmi

Drenmi Oct 6, 2017

Collaborator

There's a Node#sibling_index method you can use. 🙂

This comment has been minimized.

Copy link
@koic

koic Oct 7, 2017

Author Member

Thanks for the review. I fixed it with f9a06a2 together with the following review comment.
#4840 (comment)

# prepend M
# end
class TopLevelInclude < Cop
MSG = '`%s` is used at the top level. Use inside `class` or `module`.'

This comment has been minimized.

Copy link
@Drenmi

Drenmi Oct 6, 2017

Collaborator

Please use the annotated form for the format string, e.g.:

`%<keyword>s`

(I think someone was working on a cop for this. 🤔)

This comment has been minimized.

Copy link
@rrosenblum

rrosenblum Oct 6, 2017

Contributor

Isn't there a cop that checks for this?

This comment has been minimized.

Copy link
@bbatsov

bbatsov Oct 7, 2017

Collaborator

Yeah, there's such a cop indeed. Not sure what we set it do, though.

@bbatsov

This comment has been minimized.

Copy link
Collaborator

commented Oct 7, 2017

Btw, since when this is bad just for Rails apps?

@koic koic force-pushed the koic:add_new_rails_top_level_include_cop branch from 0cfec79 to f9a06a2 Oct 7, 2017

@koic

This comment has been minimized.

Copy link
Member Author

commented Oct 7, 2017

Yes. Outside of Rails, programmers may want to use include at the top level. For example, a script programmed at the top level without class definition, a library that intentionally want to change at the top level, etc are conceivable. For that reason I restricted the scope to app directory of Rails application. That case surely comes off the rail and breaks the responsibility of object design.

@bbatsov

This comment has been minimized.

Copy link
Collaborator

commented Oct 7, 2017

My point was that no application developer will want to do something like this most of the time, and people do write applications without Rails.

@koic

This comment has been minimized.

Copy link
Member Author

commented Oct 7, 2017

I think that's right. A use case I encountered this time was a Rails application, but I think there is a possibility that it can be applied to applications without Rails.
However, I'm wondering if this cop can be generalized for applications without file splitting rules 😟
In this PR, I started with applying it to the smallest and practical use case first.

@pocke

This comment has been minimized.

Copy link
Member

commented Oct 9, 2017

I think it's a style cop. The cop has three styles, implicit, explicit and open_class. (but I think the all options are not necessary in the first pull-request.)

# implicit
include M

# explicit
Obuject.include M

# open_class
class Object
  include M
end
@bbatsov

This comment has been minimized.

Copy link
Collaborator

commented Oct 9, 2017

That sounds reasonable to me.

@koic

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2017

I think it's a style cop. The cop has three styles

That makes sense. This looks good to me.
First of all, I'd like to make the following changes in this PR.

  • Move the department of TopLevelInclude cop from Rails to Style.
  • Remove stories related to Rails from document.

@bbatsov First This PR will target open_class style only. explicit style and inplicit style will be implemented on another occasion. WDYT?

@bbatsov

This comment has been minimized.

Copy link
Collaborator

commented Oct 9, 2017

@bbatsov First This PR will target open_class style only. explicit style and inplicit style will be implemented on another occasion. WDYT?

Fine by me.

@koic koic force-pushed the koic:add_new_rails_top_level_include_cop branch 3 times, most recently from 9de3fc9 to 204e453 Oct 10, 2017

@koic koic changed the title Add new `Rails/TopLevelInclude` cop Add new `Style/TopLevelInclude` cop Oct 10, 2017

@koic

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2017

I updated and rebased. The following is the commit that changed the department.
9de3fc9

Also I updat PR's title and commit message to Style/TopLevelInclude cop.

@koic koic force-pushed the koic:add_new_rails_top_level_include_cop branch from 204e453 to b4acb67 Oct 11, 2017

@bbatsov

This comment has been minimized.

Copy link
Collaborator

commented Oct 12, 2017

I'm just not sure about the name of the cop as it deals with more than just include.

@koic

This comment has been minimized.

Copy link
Member Author

commented Oct 13, 2017

Indeed. I considered the name Style/Mixin cop or Style/TopLevelMixin cop. Since the name Style/TopLevelMixin cop is not suitable when supporting explicit style and inplicit style in the future, I would like to make it Style/Mixin cop. However, Style/Mixin cop is a prime name so I'm worried about using it. WDYT?

@bbatsov

This comment has been minimized.

Copy link
Collaborator

commented Oct 15, 2017

Maybe Style/MixinUsage?

@koic koic force-pushed the koic:add_new_rails_top_level_include_cop branch from b4acb67 to dfc7ddb Oct 15, 2017

@koic

This comment has been minimized.

Copy link
Member Author

commented Oct 15, 2017

It looks like a very concise and clear name🌟 I changed this cop name to Style/MixinUsage cop. Thank you!

@koic koic changed the title Add new `Style/TopLevelInclude` cop Add new `Style/MixinUsage` cop Oct 15, 2017

@koic koic force-pushed the koic:add_new_rails_top_level_include_cop branch 2 times, most recently from c6385d5 to 5ab74ce Oct 16, 2017

Add new `Style/TopLevelInclude` cop
Checks that `include`, `extend` and `prepend` exists at the top level.
Using these at the top level affects the behavior of `Object`.

```console
% cat app/models/example.rb

include M

class Example < ApplicationRecord
end
```

```console
% rubocop app/models/example.rb
Inspecting 1 file
C

Offenses:

/tmp/example.rb:3:1: C: include is used at the top level. Use inside
class or module.
include M
^^^^^^^^^

1 file inspected, 1 offense detected
```

Since it is difficult to automatically determine the position of
`include` for class including multiple modules, autocorrect is not
provided.

@koic koic force-pushed the koic:add_new_rails_top_level_include_cop branch from 5ab74ce to a383739 Oct 17, 2017

@bbatsov bbatsov merged commit 57d025a into rubocop-hq:master Oct 17, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bbatsov

This comment has been minimized.

Copy link
Collaborator

commented Oct 17, 2017

👍

I forgot to reword the commit message (which mentions the old name), but we can live with this.

@koic koic deleted the koic:add_new_rails_top_level_include_cop branch Oct 17, 2017

@mpotter mpotter referenced this pull request Oct 21, 2017

Merged

Upgrade to rubocop 0.51.0 #55

5 of 5 tasks complete

@koic koic referenced this pull request Oct 30, 2017

Merged

[Fix #4885] Fix false offense detected by `Style/MixinUsage` cop #4941

10 of 11 tasks complete

koic added a commit to koic/rubocop that referenced this pull request Feb 28, 2019

[Fix rubocop-hq#4840] Fix a false positive for `Style/TrivialAccessors`
This PR fixes a false positive for `Style/TrivialAccessors` when
using reader / writer at the top level.

The following is a reproduction code.

```ruby
# example.rb
def foo
  @foo
end
```

```console
% rubocop example.rb --only Style/TrivialAccessors -a
Inspecting 1 file
C

Offenses:

example.rb:1:1: C: [Corrected] Style/TrivialAccessors: Use attr_reader
to define trivial reader methods.
def response
^^^

1 file inspected, 1 offense detected, 1 offense corrected
```

An error occurs in the auto-corrected code.

```diff
% git diff
diff --git a/example.rb b/example.rb
index b26fec0..1868b1d 100644
--- a/example.rb
+++ b/example.rb
@@ -1,5 +1,3 @@
-def response
-  @response
-end
+attr_reader :response
```

```console
% ruby example.rb
Traceback (most recent call last):
example.rb:1:in `<main>': undefined method `attr_reader' for main:Object (NoMethodError)
```

This also occurs with writer. This PR will allow these cases.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.