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

Style/AccessorGrouping: Fix sibling detection for methods with type sigs #11675

Merged

Conversation

issyl0
Copy link
Contributor

@issyl0 issyl0 commented Mar 7, 2023

  • Over in Homebrew/brew, trying to enable this cop after 008506d was released, we were seeing the following offenses which were incorrect since we can't combine attrs that follow on from typed ones.
  sig { returns(Pathname) }
  attr_reader :cached_location

  attr_reader :cache, :meta, :name, :version
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Group together all `attr_reader` attributes

Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • 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.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

@issyl0 issyl0 force-pushed the accessor-grouping-fix-sibling-detection branch from 208abd8 to e816e09 Compare March 7, 2023 21:51
- Over in `Homebrew/brew`, trying to enable this cop after
  008506d was released, we were seeing
  the following offenses which were incorrect since we can't combine
  attrs that follow on from typed ones.
- This commit ensures that sibling accessors are groupable before
  raising an offense that they should be grouped.
- Also add more tests to ensure the offense detection and subsequent
  autocorrection still works.

```
  sig { returns(Pathname) }
  attr_reader :cached_location

  attr_reader :cache, :meta, :name, :version
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Group together all `attr_reader` attributes
```
@issyl0 issyl0 force-pushed the accessor-grouping-fix-sibling-detection branch from e816e09 to 1fd778b Compare March 7, 2023 22:00
end
RUBY

expect_correction(<<~RUBY)
Copy link
Contributor Author

@issyl0 issyl0 Mar 7, 2023

Choose a reason for hiding this comment

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

I ran this (new) test on RuboCop master (v1.48.0) and the autocorrect is completely bugged in the latest release because of the sibling grouping detection. Sorry for not thinking about this sooner! 😳

  1) RuboCop::Cop::Style::AccessorGrouping when EnforcedStyle is grouped registers an offense for grouped accessors distinct from a typechecked accessor method
     Failure/Error: expect(new_source).to eq(correction)
     
       expected: "class Foo\n  extend T::Sig\n\n  sig { returns(Integer) }\n  attr_reader :one\n\n  attr_reader :two, :three, :four\nend\n"
            got: "class Foo\n  extend T::Sig\n\n  sig { returns(Integer) }\n  attr_reader :one\nend\n"
     
       (compared using ==)
     
       Diff:
       @@ -3,7 +3,5 @@
        
          sig { returns(Integer) }
          attr_reader :one
       -
       -  attr_reader :two, :three, :four
        end

@koic koic merged commit 1cd7cd2 into rubocop:master Mar 8, 2023
@koic
Copy link
Member

koic commented Mar 8, 2023

Thanks!

@issyl0 issyl0 deleted the accessor-grouping-fix-sibling-detection branch March 8, 2023 17:07
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.

None yet

2 participants