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

[Fix #8102] Consider class-length instead of block-length for Struct.new #8122

Merged
merged 1 commit into from Sep 29, 2020

Conversation

tejasbubane
Copy link
Contributor

Closes #8102


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).
  • 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.
  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

@bbatsov
Copy link
Collaborator

bbatsov commented Jun 8, 2020

I guess we should also mention this in the docs - I see we haven't done this for classes as well. Probably some examples there would also be useful.

@bbatsov
Copy link
Collaborator

bbatsov commented Jun 12, 2020

@tejasbubane Ping :-)

@tejasbubane
Copy link
Contributor Author

@bbatsov Fixed.

CHANGELOG.md Outdated Show resolved Hide resolved
@koic
Copy link
Member

koic commented Jul 26, 2020

I guess we should also mention this in the docs - I see we haven't done this for classes as well. Probably some examples there would also be useful.

@tejasbubane Can you mention this in the docs?

@bkuhlmann
Copy link

bkuhlmann commented Aug 26, 2020

@tejasbubane It looks like the conflict with the CHANGELOG.md is all that is holding this PR from being rebased onto master? Would it be possible to rebase out your conflicts? I'd be most eager to make use of this fix. By the way, thanks for fixing this. 😉

@tejasbubane
Copy link
Contributor Author

@bkuhlmann Thanks for the reminder, this slipped between the cracks.

@koic I have added documentation notes to the cops.

@tejasbubane
Copy link
Contributor Author

@bbatsov Changed.

@tejasbubane
Copy link
Contributor Author

Rebased on master

Copy link
Contributor

@marcandre marcandre left a comment

Choose a reason for hiding this comment

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

Oops, looks like I forgot to review this. Got hit again by this today, went looking for it :-)
Looks great!

Can you rebase to fix the Changelog conflict?

@tejasbubane
Copy link
Contributor Author

@marcandre Done.

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.

I wrote several small feedback, in any case this looks good overall :-)

@koic
Copy link
Member

koic commented Sep 29, 2020

Thank you always!

@tejasbubane tejasbubane deleted the fix-8102 branch September 29, 2020 14:25
@marcandre
Copy link
Contributor

❤️ thanks @tejasbubane, and @koic for the review!

koic added a commit to rubocop/rubocop-rails that referenced this pull request Sep 30, 2020
Follow rubocop/rubocop#8122.

This commit suppresses the following RuboCop's offense.

```console
% bundle exec rake
(snip)

Offenses:

lib/rubocop/cop/mixin/index_method.rb:122:80: W:
Lint/RedundantCopDisableDirective: Unnecessary disabling of
Metrics/BlockLength.
      Autocorrection = Struct.new(:match, :block_node, :leading,
      :trailing) do # rubocop:disable Metrics/BlockLength
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

187 files inspected, 1 offense detected, 1 offense auto-correctable
RuboCop failed!
```
koic added a commit to koic/rubocop that referenced this pull request Oct 1, 2020
Follow rubocop#8122.

This PR fixes the following error for `Metrics/ClassLength`
when using or assignment (`||=`).

```console
% cat example.rb
Constant ||= Struct.new {}

% bundle exec rubocop --only Layout/ClassLength -d example.rb
(snip)

Inspecting 1 file
Scanning /Users/koic/src/github.com/koic/rubocop-issues/8122/example.rb
An error occurred while Metrics/ClassLength cop was inspecting
/Users/koic/src/github.com/koic/rubocop-issues/8122/example.rb:1:0.
undefined method `class_definition?' for nil:NilClass
/Users/koic/src/github.com/rubocop-hq/rubocop/lib/rubocop/cop/metrics/class_length.rb:43:in
`on_casgn'
/Users/koic/src/github.com/rubocop-hq/rubocop/lib/rubocop/cop/commissioner.rb:99:in
`block (2 levels) in trigger_responding_cops'
/Users/koic/src/github.com/rubocop-hq/rubocop/lib/rubocop/cop/commissioner.rb:152:in
`with_cop_error_handling'
/Users/koic/src/github.com/rubocop-hq/rubocop/lib/rubocop/cop/commissioner.rb:98:in
`block in trigger_responding_cops'
/Users/koic/src/github.com/rubocop-hq/rubocop/lib/rubocop/cop/commissioner.rb:97:in
`each'
/Users/koic/src/github.com/rubocop-hq/rubocop/lib/rubocop/cop/commissioner.rb:97:in
`trigger_responding_cops'
/Users/koic/src/github.com/rubocop-hq/rubocop/lib/rubocop/cop/commissioner.rb:70:in
`on_casgn'
/Users/koic/src/github.com/rubocop-hq/rubocop-ast/lib/rubocop/ast/traversal.rb:60:in
`block in on_or_asgn'
/Users/koic/src/github.com/rubocop-hq/rubocop-ast/lib/rubocop/ast/traversal.rb:60:in
`each'
/Users/koic/src/github.com/rubocop-hq/rubocop-ast/lib/rubocop/ast/traversal.rb:60:in
`on_or_asgn'
/Users/koic/src/github.com/rubocop-hq/rubocop/lib/rubocop/cop/commissioner.rb:72:in
`on_or_asgn'
/Users/koic/src/github.com/rubocop-hq/rubocop-ast/lib/rubocop/ast/traversal.rb:14:in
`walk'
/Users/koic/src/github.com/rubocop-hq/rubocop/lib/rubocop/cop/commissioner.rb:85:in
`investigate'
```

This PR have also tweaked the highlight area of offense to more eligible
location for count the class block length.

Before:

```ruby
Foo = Class.new do
^^^ Class has too many lines. [6/5]
```

After:

```ruby
Foo = Class.new do
      ^^^^^^^^^^^^ Class has too many lines. [6/5]
```

I haven't added the fixing to the changelog because rubocop#8122 hasn't been released yet.
@koic koic mentioned this pull request Oct 1, 2020
8 tasks
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.

Struct.new do and Metrics/BlockLength vs Metrics/ClassLength
5 participants