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

build(deps-dev): bump rubocop from 1.67.0 to 1.68.0 in /Library/Homebrew #18682

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

dependabot[bot]
Copy link
Contributor

@dependabot dependabot bot commented on behalf of github Oct 31, 2024

Bumps rubocop from 1.67.0 to 1.68.0.

Release notes

Sourced from rubocop's releases.

RuboCop 1.68

New features

Bug fixes

  • #13401: Fix a false negative for Style/RedundantLineContinuation when there is a line continuation at the EOF. (@​koic)
  • #13368: Fix an incorrect autocorrect for Naming/BlockForwarding with Style/ExplicitBlockArgument. (@​koic)
  • #13391: Fix deserialization of unknown encoding offenses. (@​earlopain)
  • #13348: Ensure Style/BlockDelimiters autocorrection does not move other code between the block and comment. (@​dvandersluis)
  • #13382: Fix an error during error handling for custom ruby extractors when the extractor is a class. (@​earlopain)
  • #13309: Fix a false negative for Lint/UselessAssignment cop when there is a useless assignment followed by a block. (@​pCosta99)
  • #13255: Fix false negatives for Style/MapIntoArray when using non-splatted arguments. (@​vlad-pisanov)
  • #13356: Fix a false positive for Layout/SpaceBeforeBrackets when there is a dot before []=. (@​earlopain)
  • #13365: Fix false positives for Lint/SafeNavigationConsistency when using safe navigation on the LHS with operator method on the RHS of &&. (@​koic)
  • #13390: Fix false positives for Style/GuardClause when using a local variable assigned in a conditional expression in a branch. (@​koic)
  • #13337: Fix false positives for Style/RedundantLineContinuation when required line continuations for && is used with an assignment after a line break. (@​koic)
  • #13387: Fix false positives in Style/RedundantParentheses when parentheses are used around method chain with do...end block in keyword argument. (@​koic)
  • #13341: Fix false positives for Lint/SafeNavigationChain when a safe navigation operator is used with a method call as the RHS operand of && for the same receiver. (@​koic)
  • #13324: Fix --disable-uncorrectable to not insert a comment inside a string continuation. (@​dvandersluis)
  • #13364: Fix incorrect autocorrect with Lint/UselessAssignment a multiple assignment or for contains an inner assignment. (@​dvandersluis)
  • #13353: Fix an incorrect autocorrect for Style/BlockDelimiters when EnforcedStyle: semantic is set and used with Layout/SpaceInsideBlockBraces. (@​koic)
  • #13361: Fix false positives for Style/RedundantInterpolationUnfreeze and Style/RedundantFreeze when strings contain interpolated global, instance, and class variables. (@​vlad-pisanov)
  • #13343: Prevent Layout/LineLength from breaking up a method with arguments chained onto a heredoc delimiter. (@​dvandersluis)
  • #13374: Return exit code 0 with --display-only-correctable and --display-only-safe-correctable when no offenses are displayed. (@​dvandersluis)
  • #13193: Fix false positive in Style/MultipleComparison when ComparisonsThreshold exceeds 2. (@​fatkodima,@​vlad-pisanov)
  • #13325: Fix an incorrect autocorrect for Lint/NonAtomicFileOperation when using a postfix unless for file existence checks before creating a file, in cases with Dir.mkdir. ([@​kotaro0522][])
  • #13397: Update PercentLiteralCorrector to be able to write pairs of delimiters without excessive escaping. (@​dvandersluis)
  • #13336: Update Style/SafeNavigation to not autocorrect if the RHS of an and node is an or node. (@​dvandersluis)
  • #13378: When removing parens in Style/TernaryParentheses with a send node condition, ensure its arguments are parenthesized. (@​dvandersluis)

Changes

  • #13347: When running rubocop -V, show the analysis Ruby version of the current directory. (@​earlopain)

... (truncated)

Changelog

Sourced from rubocop's changelog.

1.68.0 (2024-10-31)

New features

Bug fixes

  • #13401: Fix a false negative for Style/RedundantLineContinuation when there is a line continuation at the EOF. ([@​koic][])
  • #13368: Fix an incorrect autocorrect for Naming/BlockForwarding with Style/ExplicitBlockArgument. ([@​koic][])
  • #13391: Fix deserialization of unknown encoding offenses. ([@​earlopain][])
  • #13348: Ensure Style/BlockDelimiters autocorrection does not move other code between the block and comment. ([@​dvandersluis][])
  • #13382: Fix an error during error handling for custom ruby extractors when the extractor is a class. ([@​earlopain][])
  • #13309: Fix a false negative for Lint/UselessAssignment cop when there is a useless assignment followed by a block. ([@​pCosta99][])
  • #13255: Fix false negatives for Style/MapIntoArray when using non-splatted arguments. ([@​vlad-pisanov][])
  • #13356: Fix a false positive for Layout/SpaceBeforeBrackets when there is a dot before []=. ([@​earlopain][])
  • #13365: Fix false positives for Lint/SafeNavigationConsistency when using safe navigation on the LHS with operator method on the RHS of &&. ([@​koic][])
  • #13390: Fix false positives for Style/GuardClause when using a local variable assigned in a conditional expression in a branch. ([@​koic][])
  • #13337: Fix false positives for Style/RedundantLineContinuation when required line continuations for && is used with an assignment after a line break. ([@​koic][])
  • #13387: Fix false positives in Style/RedundantParentheses when parentheses are used around method chain with do...end block in keyword argument. ([@​koic][])
  • #13341: Fix false positives for Lint/SafeNavigationChain when a safe navigation operator is used with a method call as the RHS operand of && for the same receiver. ([@​koic][])
  • #13324: Fix --disable-uncorrectable to not insert a comment inside a string continuation. ([@​dvandersluis][])
  • #13364: Fix incorrect autocorrect with Lint/UselessAssignment a multiple assignment or for contains an inner assignment. ([@​dvandersluis][])
  • #13353: Fix an incorrect autocorrect for Style/BlockDelimiters when EnforcedStyle: semantic is set and used with Layout/SpaceInsideBlockBraces. ([@​koic][])
  • #13361: Fix false positives for Style/RedundantInterpolationUnfreeze and Style/RedundantFreeze when strings contain interpolated global, instance, and class variables. ([@​vlad-pisanov][])
  • #13343: Prevent Layout/LineLength from breaking up a method with arguments chained onto a heredoc delimiter. ([@​dvandersluis][])
  • #13374: Return exit code 0 with --display-only-correctable and --display-only-safe-correctable when no offenses are displayed. ([@​dvandersluis][])
  • #13193: Fix false positive in Style/MultipleComparison when ComparisonsThreshold exceeds 2. ([@​fatkodima][],[@​vlad-pisanov][])
  • #13325: Fix an incorrect autocorrect for Lint/NonAtomicFileOperation when using a postfix unless for file existence checks before creating a file, in cases with Dir.mkdir. ([@​kotaro0522][])
  • #13397: Update PercentLiteralCorrector to be able to write pairs of delimiters without excessive escaping. ([@​dvandersluis][])
  • #13336: Update Style/SafeNavigation to not autocorrect if the RHS of an and node is an or node. ([@​dvandersluis][])
  • #13378: When removing parens in Style/TernaryParentheses with a send node condition, ensure its arguments are parenthesized. ([@​dvandersluis][])

Changes

  • #13347: When running rubocop -V, show the analysis Ruby version of the current directory. ([@​earlopain][])
Commits
  • 7d35ef7 Cut 1.68
  • 3033deb Update Changelog
  • 37e9e5f [Fix #12140] Add new Style/CombinableDefined cop.
  • f8aa27f Fix a false negative for Style/RedundantLineContinuation
  • d033a5e Merge pull request #13400 from Earlopain/offense-cop-name-docs
  • faaa349 Fix docs for Offense.cop_name
  • 85f9405 Fix deserialization of unknown encoding offenses
  • d499d80 [Fix #13387] Fix false positives for Style/RedundantParentheses
  • 3e855b0 Merge pull request #13399 from dvandersluis/fix-and-offense-typo
  • 1f58513 Fix typos and offense instead of an offense.
  • Additional commits viewable in compare view

Dependabot compatibility score

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.


Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

  • @dependabot rebase will rebase this PR
  • @dependabot recreate will recreate this PR, overwriting any edits that have been made to it
  • @dependabot merge will merge this PR after your CI passes on it
  • @dependabot squash and merge will squash and merge this PR after your CI passes on it
  • @dependabot cancel merge will cancel a previously requested merge and block automerging
  • @dependabot reopen will reopen this PR if it is closed
  • @dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
  • @dependabot show <dependency name> ignore conditions will show all of the ignore conditions of the specified dependency
  • @dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)

@dependabot dependabot bot added dependencies Bumping Gemfile dependencies ruby Pull requests that update Ruby code labels Oct 31, 2024
@dependabot dependabot bot force-pushed the dependabot/bundler/Library/Homebrew/rubocop-1.68.0 branch from b65ac2a to 8a3947e Compare October 31, 2024 19:04
@apainintheneck
Copy link
Contributor

Most of the new cops seem reasonable but the Style/SafeNavigationChainLength limit of two 2 safe navigation operations in a row seems kind of arbitrary. I'm inclined to ignore it since some of the offending examples don't have great alternatives.

@dduugg
Copy link
Member

dduugg commented Nov 2, 2024

Most of the new cops seem reasonable but the Style/SafeNavigationChainLength limit of two 2 safe navigation operations in a row seems kind of arbitrary. I'm inclined to ignore it since some of the offending examples don't have great alternatives.

That's a fair point, but I'd be willing to try manually resolving these (since I've been writing Ruby since the pre-&. releases 😬).

For something like formula.stable&.patches&.select(&:external?)&.map(&:resource)&.map(&:version), there are a couple readability issues. I don't like the mix of nil-forwarding (&.) and block-conversion (&:). It's also usually the case that only the first &. represents a nilable method, and it's better to write something closer to:

formula.stable.patches.select(&:external?).map(&:resource).map(&:version) if formula.stable

(perhaps with an lvar for efficiency)

@apainintheneck
Copy link
Contributor

Good point about the similarity of &. and &:. I hadn't thought of that one. I think it'll be easier to assess whether we like the new syntax after trying to correct it so I say give it a try. Even if it looks a little weird, it doesn't happen that much so maybe it's not worth disabling the cop to keep 5 lines of code more idiomatic.

Bumps [rubocop](https://github.com/rubocop/rubocop) from 1.67.0 to 1.68.0.
- [Release notes](https://github.com/rubocop/rubocop/releases)
- [Changelog](https://github.com/rubocop/rubocop/blob/master/CHANGELOG.md)
- [Commits](rubocop/rubocop@v1.67.0...v1.68.0)

---
updated-dependencies:
- dependency-name: rubocop
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
@dduugg dduugg force-pushed the dependabot/bundler/Library/Homebrew/rubocop-1.68.0 branch from bc089d9 to c5e2c4b Compare November 2, 2024 22:12
@dduugg dduugg force-pushed the dependabot/bundler/Library/Homebrew/rubocop-1.68.0 branch from c5e2c4b to 9203419 Compare November 2, 2024 22:17
Library/Homebrew/formula.rb Show resolved Hide resolved
Library/Homebrew/tap.rb Show resolved Hide resolved
Library/Homebrew/utils/spdx.rb Show resolved Hide resolved
@dduugg
Copy link
Member

dduugg commented Nov 2, 2024

@apainintheneck have a look. I think I slightly prefer the new versions (assuming they're equivalent), but I'm not sure I want to make future contributors go through these gymnastics…

Copy link
Contributor

@apainintheneck apainintheneck left a comment

Choose a reason for hiding this comment

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

These changes look good to me. I'm not too worried about contributors running into this linter error since the offending pattern is pretty rare throughout the codebase.

Library/Homebrew/cleanup.rb Outdated Show resolved Hide resolved
Library/Homebrew/cask/cask.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/livecheck.rb Show resolved Hide resolved
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.

Thanks @dduugg, much nicer!

@MikeMcQuaid
Copy link
Member

It's also usually the case that only the first &. represents a nilable method, and it's better to write something closer to

I agree with this 👍🏻

@MikeMcQuaid
Copy link
Member

@dduugg Some changes needed in homebrew/cask here CC @Homebrew/cask folks if you're able to fix these up 🙇🏻

@SMillerDev
Copy link
Member

It seems all of those are livechecks, so unfortunately I can't help because I'm not too confident in those.

@samford
Copy link
Member

samford commented Nov 4, 2024

It seems all of those are livechecks

From a quick glance, all of the cask offenses are related to safe navigation chains longer than two calls. All but two are from Xml strategy blocks, where we do things like this (from the alfred cask):

strategy :xml do |xml|
  version = xml.elements["//key[text()='version']"]&.next_element&.text&.strip
  build = xml.elements["//key[text()='build']"]&.next_element&.text&.strip
  next if version.blank? || build.blank?

  "#{version},#{build}"
end

According to the REXML documentation, #elements, #next_element, and #text can all return nil.

Arbitrarily limiting safe navigation chains to two calls would just force us to add unnecessary intermediate variables or otherwise complicate this logic simply to appease RuboCop. Something like:

strategy :xml do |xml|
  version_value = xml.elements["//key[text()='version']"]&.next_element
  version = version_value.text&.strip if version_value
  build_value = xml.elements["//key[text()='build']"]&.next_element
  build = build_value.text&.strip if build_value
  next if version.blank? || build.blank?

  "#{version},#{build}"
end

We could technically maintain the same number of lines if we move the #strip calls into the return string but this feels a bit forced:

strategy :xml do |xml|
  version = xml.elements["//key[text()='version']"]&.next_element&.text
  build = xml.elements["//key[text()='build']"]&.next_element&.text
  next if version.blank? || build.blank?

  "#{version.strip},#{build.strip}"
end

That said, this is just one simple example and there are more complicated Xml strategy blocks where we would have to create additional intermediate variables.

I'm sure there are other alternative solutions to resolve this but my takeaway is that this restriction doesn't feel like a net positive. I'm in favor of minimizing safe navigation chains when we can do so without introducing unnecessary variables or organizing the code in an unusual way (just to avoid the offense) but that's not the case with these cask examples.

@MikeMcQuaid
Copy link
Member

@samford personally I do very much prefer the code style of both of the latter examples, particularly the latter, to the first/current one.

According to the REXML documentation, #elements, #next_element, and #text can all return nil.

For example: this is not obvious from the existing code but is made obvious from the new code.

This also feels like the case (longer-term, should not block this PR) where we would be better creating our own DSLs/wrappers around the existing APIs.

@samford
Copy link
Member

samford commented Nov 5, 2024

After reading the description for the Style/SafeNavigationChainLength check and your comment, I now understand that the benefit of limiting safe navigation chains is that it can make it clearer what may be nil. If this makes unfamiliar code easier to understand, then it may be an okay tradeoff.

I'll work on addressing the ~50 offending strategy blocks in homebrew/cask tomorrow.

This also feels like the case (longer-term, should not block this PR) where we would be better creating our own DSLs/wrappers around the existing APIs.

Just so I understand, are you referring to the REXML APIs? If so, I agree that there's room for improvement there. I've already created an #element_text method in the livecheck Xml strategy (to avoid having to repeat some boilerplate code everywhere) but we don't use it in Xml strategy blocks. Part of the reason is because the namespaced method call is sometimes longer than the code we would be replacing (Homebrew::Livecheck::Strategy::Xml.element_text(...)) but that's something we can address if we want to go that route.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Bumping Gemfile dependencies ruby Pull requests that update Ruby code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants