-
-
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
rubocop: order uninstall/zap methods #16377
rubocop: order uninstall/zap methods #16377
Conversation
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.
Makes sense to me! Rerequest review when no longer draft.
49c51bf
to
4e18311
Compare
I'm having some issues writing tests for this. I would appreciate any help, but the rest of the PR should be ready. |
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.
Code looks good but needs some tests. I'd suggest copy-pasting existing tests and trying to modify or asking @issyl0 for help in Slack.
@razvanazamfirei Below is an initial run through a test spec for this. Note that these are currently failing due to some potential issues with how whitespace is taken into account in the auto-corrector. There's also a test that takes into account inline comments which is currently failing. Expand for Code`/Library/Homebrew/test/rubocops/cask/uninstall_methods_order_spec.rb` # frozen_string_literal: true
require "rubocops/rubocop-cask"
describe RuboCop::Cop::Cask::UninstallMethodsOrder, :config do
it "accepts when all stanzas are in order" do
expect_no_offenses <<~RUBY
cask 'foo' do
uninstall early_script: {
executable: "foo.sh",
args: ["--unattended"],
},
launchctl: "com.example.foo",
quit: "com.example.foo",
signal: ["TERM", "com.example.foo"],
login_item: "FooApp",
kext: "com.example.foo",
script: {
executable: "foo.sh",
args: ["--unattended"],
},
pkgutil: "com.example.foo",
delete: "~/Library/Preferences/com.example.foo",
trash: "~/Library/Preferences/com.example.foo",
rmdir: "~/Library/Foo"
zap early_script: {
executable: "foo.sh",
args: ["--unattended"],
},
launchctl: "com.example.foo",
quit: "com.example.foo",
signal: ["TERM", "com.example.foo"],
login_item: "FooApp",
kext: "com.example.foo",
script: {
executable: "foo.sh",
args: ["--unattended"],
},
pkgutil: "com.example.foo",
delete: "~/Library/Preferences/com.example.foo",
trash: "~/Library/Preferences/com.example.foo",
rmdir: "~/Library/Foo"
end
RUBY
end
it "reports an offense when uninstall methods are out of order" do
expect_offense <<~CASK
cask 'foo' do
uninstall quit: "com.example.foo",
^^^^ `quit` method out of order
launchctl: "com.example.foo"
^^^^^^^^^ `launchctl` method out of order
end
CASK
expect_correction <<~CASK
cask 'foo' do
uninstall launchctl: "com.example.foo",
quit: "com.example.foo"
end
CASK
end
it "reports an offense when zap methods are out of order" do
expect_offense <<~CASK
cask 'foo' do
zap rmdir: "/Library/Foo",
^^^^^ `rmdir` method out of order
trash: "com.example.foo"
^^^^^ `trash` method out of order
end
CASK
expect_correction <<~CASK
cask 'foo' do
zap trash: "com.example.foo",
rmdir: "/Library/Foo"
end
CASK
end
it "keeps associated comments when auto-correcting" do
expect_offense <<~CASK
cask 'foo' do
uninstall quit: "com.example.foo", # comment on same line
^^^^ `quit` method out of order
launchctl: "com.example.foo"
^^^^^^^^^ `launchctl` method out of order
end
CASK
expect_correction <<~CASK
cask 'foo' do
uninstall launchctl: "com.example.foo",
quit: "com.example.foo" # comment on same line
end
CASK
end
it "registers an offense when inside an `on_os` block" do
expect_offense <<~CASK
cask "foo" do
on_catalina do
uninstall trash: "com.example.foo",
^^^^^ `trash` method out of order
launchctl: "com.example.foo"
^^^^^^^^^ `launchctl` method out of order
end
on_ventura do
uninstall quit: "com.example.foo",
^^^^ `quit` method out of order
launchctl: "com.example.foo"
^^^^^^^^^ `launchctl` method out of order
end
end
CASK
expect_correction <<~CASK
cask "foo" do
on_catalina do
uninstall launchctl: "com.example.foo",
trash: "com.example.foo"
end
on_ventura do
uninstall launchctl: "com.example.foo",
quit: "com.example.foo"
end
end
CASK
end
end |
4e18311
to
171a366
Compare
Thank you so much, @bevanjkay! Let me merge the tests and I'll fix the inline comments issue! |
c92a877
to
898d8f0
Compare
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.
Code looking good now (once CI failures addressed)!
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
898d8f0
to
708792d
Compare
Co-authored-by: Bevan Kay <email@bevankay.me>
71f0ef5
to
4958710
Compare
@razvanazamfirei I haven't checked for any occurrences in the repo, so this may not be a current issue. But what happens if there is a comment between two uninstall/zap keys.
|
@bevanjkay, this seems to be a problem only in one cask: uninstall signal: [
["TERM", "com.every.thing.controller#{version.major}"],
["TERM", "com.every.thing.bin"],
],
# Something about launchctl not relevant to anything else
launchctl: "com.every.thing.agent",
kext: "com.every.thing.driver",
delete: "/Library/EverythingHelperTools" would be corrected to: # Something about launchctl not relevant to anything else
uninstall launchctl: "com.every.thing.agent",
signal: [
["TERM", "com.every.thing.controller#{version.major}"],
["TERM", "com.every.thing.bin"],
],
kext: "com.every.thing.driver",
delete: "/Library/EverythingHelperTools" |
4958710
to
6d6f5fe
Compare
6d6f5fe
to
82cdf27
Compare
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.
Looks great, thanks for the perseverance here, the comments handling has a tendency to 🤯.
Thanks for the patience, @issyl0! :) |
Thanks again @razvanazamfirei! |
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?This PR introduces a new RuboCop rule to ensure a standardized order of uninstall/zap methods. The changes involve updating the constants to include a predefined order for uninstall methods and adding a new cop (UninstallMethodsOrder) to enforce this order.
Note: Our documentation currently conflicts with this implementation. Our documentation recommends ordering methods alphabetically. This cop orders methods based on the order of execution. I think both are reasonable and I would be happy to change it to alphabetical.
This should work nicely with: #16365