rubocop: order uninstall/zap methods

[razvanazamfirei]

• 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?

---

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

[MikeMcQuaid]

Makes sense to me! Rerequest review when no longer draft.

[razvanazamfirei]

I'm having some issues writing tests for this. I would appreciate any help, but the rest of the PR should be ready.

[MikeMcQuaid]

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.

[bevanjkay]

@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.
This doesn't show up when you run brew style because each offense is then picked up by the white-space cop.

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

[razvanazamfirei]

Thank you so much, @bevanjkay! Let me merge the tests and I'll fix the inline comments issue!

[MikeMcQuaid]

Code looking good now (once CI failures addressed)!

[github-actions]

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.

[bevanjkay]

@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.

uninstall quit:  "com.foo.bar",
          # comment here
          trash: "..."

[razvanazamfirei]

@bevanjkay, this seems to be a problem only in one cask: jupyterlab. The comment will be overwritten. I'm not sure if the complexity of handling that case is worth it. It also has the potential to create confusing situations.

  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"

[issyl0]

Looks great, thanks for the perseverance here, the comments handling has a tendency to 🤯.

[razvanazamfirei]

Thanks for the patience, @issyl0! :)

[MikeMcQuaid]

Thanks again @razvanazamfirei!