-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
base: master
Are you sure you want to change the base?
build(deps-dev): bump rubocop from 1.67.0 to 1.68.0 in /Library/Homebrew #18682
Conversation
b65ac2a
to
8a3947e
Compare
Most of the new cops seem reasonable but the |
That's a fair point, but I'd be willing to try manually resolving these (since I've been writing Ruby since the pre- For something like
(perhaps with an lvar for efficiency) |
Good point about the similarity of |
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]>
bc089d9
to
c5e2c4b
Compare
c5e2c4b
to
9203419
Compare
@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… |
There was a problem hiding this 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.
Co-authored-by: Kevin <[email protected]>
There was a problem hiding this 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!
I agree with this 👍🏻 |
@dduugg Some changes needed in homebrew/cask here CC @Homebrew/cask folks if you're able to fix these up 🙇🏻 |
It seems all of those are livechecks, so unfortunately I can't help because I'm not too confident in those. |
From a quick glance, all of the cask offenses are related to safe navigation chains longer than two calls. All but two are from 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 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 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 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. |
@samford personally I do very much prefer the code style of both of the latter examples, particularly the latter, to the first/current one.
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. |
After reading the description for the I'll work on addressing the ~50 offending
Just so I understand, are you referring to the |
Bumps rubocop from 1.67.0 to 1.68.0.
Release notes
Sourced from rubocop's releases.
... (truncated)
Changelog
Sourced from rubocop's changelog.
Commits
7d35ef7
Cut 1.683033deb
Update Changelog37e9e5f
[Fix #12140] Add newStyle/CombinableDefined
cop.f8aa27f
Fix a false negative forStyle/RedundantLineContinuation
d033a5e
Merge pull request #13400 from Earlopain/offense-cop-name-docsfaaa349
Fix docs forOffense.cop_name
85f9405
Fix deserialization of unknown encoding offensesd499d80
[Fix #13387] Fix false positives forStyle/RedundantParentheses
3e855b0
Merge pull request #13399 from dvandersluis/fix-and-offense-typo1f58513
Fix typosand offense
instead ofan offense
.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)