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

rubocops/components_redundancy: stable/head block removal #16413

Merged
merged 1 commit into from Jan 15, 2024

Conversation

cho-m
Copy link
Member

@cho-m cho-m commented Dec 30, 2023

When a stable do or head do block only contains url, sha256, mirror, and/or version, then the block should be removed.

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

@cho-m cho-m force-pushed the rubocop-remove-stable-head-block branch from 8431c4e to 573695b Compare December 30, 2023 19:33
unless stable_block.body.nil?
child_nodes = stable_block.body.begin_type? ? stable_block.body.child_nodes : [stable_block.body]
if child_nodes.all? { |n| n.send_type? && STABLE_BLOCK_METHODS.include?(n.method_name) }
problem "`stable do` should not be present with only #{STABLE_BLOCK_METHODS.join("/")}"
Copy link
Member

Choose a reason for hiding this comment

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

Feels like there's a better way to word this but I don't have any suggestions. Maybe worth mentioning it's redundant rather than broken.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Looks good to me! I don't think wording change is necessary as "redundant not broken" applies to most of the RuboCops. I'd love to see an auto-correct as a follow-up PR and a test before this is merged.

@cho-m cho-m force-pushed the rubocop-remove-stable-head-block branch from 573695b to abb3e33 Compare January 12, 2024 19:16
When a `stable do` or `head do` block only contains `url`, `sha256`,
`mirror`, and/or `version`, then the block should be removed.
@cho-m cho-m force-pushed the rubocop-remove-stable-head-block branch from abb3e33 to b4657e1 Compare January 13, 2024 00:07
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @cho-m!

@MikeMcQuaid MikeMcQuaid merged commit 594ed2a into Homebrew:master Jan 15, 2024
24 checks passed
@github-actions github-actions bot added the outdated PR was locked due to age label Feb 15, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2024
@cho-m cho-m deleted the rubocop-remove-stable-head-block branch March 5, 2024 02:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants