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 detection of Sorbet sig {} blocks #11650

Merged

Conversation

issyl0
Copy link
Contributor

@issyl0 issyl0 commented Mar 1, 2023

  • Sorbet sig { returns(Integer) } annotations above attr_reader methods should be ignored after 9921cdc df7c4cd, per the docs, but they weren't being (see the test I added here for evidence).
  • Sorbet sig {} constructs are blocks (hence the braces), not send_type? directly.
  • Hence, we now check if the previous expression is a block and if so, descend into its child nodes until we find a send_type, then use that to determine if the accessor is groupable.
  • Relates to Style/AccessorGrouping false positives with sorbet-runtime type signatures #11596, but that issue is already closed.

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 fix-accessor-grouping-cop-for-sorbet-blocks branch from 9dce847 to bb70257 Compare March 1, 2023 22:57
@issyl0 issyl0 marked this pull request as ready for review March 1, 2023 23:00
@issyl0 issyl0 force-pushed the fix-accessor-grouping-cop-for-sorbet-blocks branch from bb70257 to ff1ce4c Compare March 1, 2023 23:03
@issyl0
Copy link
Contributor Author

issyl0 commented Mar 1, 2023

Force-pushed to add backticks to the cop name in the changelog.

@@ -90,6 +90,13 @@ def previous_line_comment?(node)

def groupable_accessor?(node)
return true unless (previous_expression = node.left_siblings.last)

if previous_expression.block_type?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be good to add a small comment here explaining what this bit of code targets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. Done!

annotation_method :one
attr_reader :one

annotation_method :two
attr_reader :two

sig { returns(Integer) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can also mention such an example explicitly in the cop's description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done!

- Sorbet `sig { returns(Integer) }` annotations above `attr_reader`
  methods should be ignored after
  9921cdc, per the docs, but they
  weren't being.
- Sorbet `sig {}` constructs are blocks (hence the braces), not
  `send_type?` directly.
- Hence, we now check if the previous expression is a block and if so,
  descend into its child nodes until we find a `send_type`, then use
  that to determine if the accessor is groupable.
- Relates to rubocop#11596.
@issyl0 issyl0 force-pushed the fix-accessor-grouping-cop-for-sorbet-blocks branch from ff1ce4c to 7fa0439 Compare March 2, 2023 13:44
@bbatsov bbatsov merged commit 008506d into rubocop:master Mar 3, 2023
@bbatsov
Copy link
Collaborator

bbatsov commented Mar 3, 2023

Thanks!

@issyl0 issyl0 deleted the fix-accessor-grouping-cop-for-sorbet-blocks branch March 3, 2023 08:55
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