Add a new RuboCop for alphabetizing `zap trash` array elements #16365

issyl0 · 2023-12-19T23:43:45Z

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?

Fixes rubocop/cask: sort zap stanza's trash array alphabetically #16323.
Previously this was being done manually by Cask maintainers.
While we're here, enforce that the zap trash path is not in [] if it only contains a single element.

- Part of issue 16323. - Previously this was being done manually by Cask maintainers. - While we're here, enforce that the `zap trash` path is not in `[]` if it only contains a single element. - This is buggy on actual Casks, hence the draft PR.

MikeMcQuaid

Approach looks good so far!

p-linnane · 2023-12-20T19:32:03Z

Very excited to see this, as it's something we often have to fix manually on people's PR's. Great job @issyl0!

p-linnane · 2023-12-20T19:32:31Z

cc @bevanjkay @krehel @miccal @razvanazamfirei as they often deal with this issue as well.

krehel · 2023-12-20T19:34:27Z

LOVE. Been on my mind for a while. Awesome to see this @issyl0!

Library/Homebrew/rubocops/cask/array_alphabetization.rb

- Interpolating the version into a path is a common pattern, but the interpolations trip up the alphabetization autocorrect quite spectacularly, so let's ignore them (for now?).

- Use `sort_by` to sort the array, rather than comparing each element to the next. - This doesn't error with complaints about clobbering at all when run on `homebrew/cask`, hurray. And it also handles interpolations correctly, rather than ignoring them. Co-authored-by: Bevan Kay <email@bevankay.me>

bevanjkay · 2023-12-22T00:46:26Z

@issyl0 I think it is okay and probably preferred to run this on all the arrays within the zap and trash stanzas. Except signal as it can contain an array of arrays, so may be problematic.
I should have thought more about this in the initial issue.

We also need to skip script and early_script as they can contain arrays with ordered arguments.
EDIT: it looks like it's actually the argsand input keys that needs to be skipped - we will need to confirm if there is any additional keys that need to be skipped, but they are the two that I perceive that would be problematic if re-ordered.

bevanjkay · 2024-01-01T12:13:25Z

Reflecting on this some more, it may not be possible to re-sort the arrays the way that I had proposed earlier, as there may not be any way to preserve the alignment of inline comments?

issyl0 · 2024-01-01T23:25:22Z

Reflecting on this some more, it may not be possible to re-sort the arrays the way that I had proposed earlier, as there may not be any way to preserve the alignment of inline comments?

How many casks have in-line comments? Or is it a common thing?

issyl0 · 2024-01-01T23:26:12Z

I've not forgotten about this, I just had a well-earned festive break. 😁 Back to it this week!

bevanjkay · 2024-01-01T23:33:14Z

@issyl0 Found some occurrences of comments within an array

https://github.com/Homebrew/homebrew-cask/blob/5dee9499ff02c532b7f5adf82607f78c3fd72d01/Casks/m/mplabx-ide.rb#L60-L63
https://github.com/Homebrew/homebrew-cask/blob/5dee9499ff02c532b7f5adf82607f78c3fd72d01/Casks/f/firefox.rb#L256-L257
https://github.com/Homebrew/homebrew-cask/blob/5dee9499ff02c532b7f5adf82607f78c3fd72d01/Casks/g/gpg-suite-pinentry.rb#L145
https://github.com/Homebrew/homebrew-cask/blob/5dee9499ff02c532b7f5adf82607f78c3fd72d01/Casks/j/jupyterlab.rb#L22
https://github.com/Homebrew/homebrew-cask/blob/5dee9499ff02c532b7f5adf82607f78c3fd72d01/Casks/r/rancher.rb#L24-L27

razvanazamfirei · 2024-01-02T20:53:57Z

@issyl0 @bevanjkay, I'm dealing with the same issue with comments in #16377. If there's a more general solution to handling comments, I'd love to know. I'll also be working on figuring it out.

issyl0 · 2024-01-02T23:33:43Z

Thanks for the feedback, all! This now scans uninstall as well as zap. Next up:

Comment handling (unless Razvan gets there first, which I'm not-so-secretly hoping for 😉).
Sense checking this on real Casks.

issyl0 · 2024-01-12T17:52:42Z

It seems like @razvanazamfirei figured out how to preserve comments! So I'll take a look at that this weekend and see if I can do similar here.

cho-m · 2024-01-16T14:58:23Z

What about casks like mactex which group by application, i.e.

  zap trash: [
        "/usr/local/texlive/texmf-local",
        "~/Library/texlive",
        # TexShop:
        "~/Library/Application Support/com.apple.sharedfilelist/com.apple.LSSharedFileList.ApplicationRecentDocuments/texshop.sfl*",
        "~/Library/Application Support/TeXShop",
        "~/Library/Caches/com.apple.helpd/Generated/TeXShop Help*",
        "~/Library/Caches/TeXShop",
        "~/Library/HTTPStorages/TeXShop",
        "~/Library/Preferences/TeXShop.plist",
        "~/Library/TeXShop",
        "~/Library/WebKit/TeXShop",
        # BibDesk:
        "~/Library/Application Support/BibDesk",
        "~/Library/Caches/com.apple.helpd/Generated/edu.ucsd.cs.mmccrack.bibdesk.help*",

In some ways, this could be more readable for Casks that install multiple apps and have a long zap list.

bevanjkay · 2024-01-17T04:47:42Z

@cho-m How many examples split the zap like that?
I believe it would be the exception to the rule by a very large majority.

cho-m · 2024-01-17T05:49:20Z

@cho-m How many examples split the zap like that? I believe it would be the exception to the rule by a very large majority.

I expect that to be the case but haven't fully checked.

Other examples of comment block groups I searched for are minor and probably can be replaced by alphabetizing and/or a different comment:

squirrel has a group for some zap entries relating to old version:

  zap trash: [
    "~/Library/Caches/im.rime.inputmethod.Squirrel",
    "~/Library/Preferences/im.rime.inputmethod.Squirrel.plist",
    # Data for older versions (< 0.10.0)
    "~/Library/Caches/com.googlecode.rimeime.inputmethod.Squirrel",
    "~/Library/Preferences/com.googlecode.rimeime.inputmethod.Squirrel.plist",
  ]

Not entirely sure what meaning on little-snitch zap comment group is

  zap trash: [
        "/Library/Application Support/Objective Development/Little Snitch",
        "/Library/Caches/at.obdev.LittleSnitchConfiguration",
        ...
        "~/Library/WebKit/at.obdev.LittleSnitchConfiguration",
        # These kext's should not be uninstalled by Cask
        "/Library/Extensions/LittleSnitch.kext",
        "/Library/StagedExtensions/Library/Extensions/LittleSnitch.kext",
      ],

An odd one is lark and feishu which have same zap. Didn't really check how those casks are set up:

  zap trash: [
    # feishu
    "~/Library/Caches/com.bytedance.lark.helper",
    "~/Library/Preferences/com.bytedance.lark.helper.plist",
    # lark
    "~/Library/Caches/com.electron.lark.helper",
    "~/Library/Preferences/com.electron.lark.helper.plist",
    # both
    "~/Library/Caches/com.electron.lark",
    "~/Library/Saved Application State/com.electron.lark.savedState",
  ]

bevanjkay · 2024-01-17T06:12:03Z

@cho-m For the last one, if the comment is necessary it could go in-line, if we can get the comments to stay. little-snitch the comment could go outside of the block. squirrel probably doesn't require the comment. So I think we're okay to move forward, once we can get in-line comments working

issyl0 · 2024-01-20T16:02:13Z

Massive thanks to @bevanjkay here, this is finally ready for review. Hoping we can get this over the line soon! I've run the autofixes on Homebrew/homebrew-cask (sorry for the massive PRs), the PRs are linked above.

``` 1) Cask::Cask#to_h when loaded from cask file returns expected hash Failure/Error: expect(JSON.pretty_generate(hash)).to eq(expected_json) Diff: @@ -28,9 +28,7 @@ "uninstall": [ { "launchctl": "com.every.thing.agent", - "delete": [ - "/Library/EverythingHelperTools" - ], + "delete": "/Library/EverythingHelperTools", "kext": "com.every.thing.driver", "signal": [ [ @@ -103,7 +101,7 @@ ], "ruby_source_path": "Casks/everything.rb", "ruby_source_checksum": { - "sha256": "b2707d1952f02c3fa566b7ad2a707a847a959d36f51d3dee642dbe5deec12f27" + "sha256": "0c4af571cce1632fc6a3dcf3e75ba82a3283077ef12399428192c26f9d6f779b" } } # ./test/cask/cask_spec.rb:225:in `block (4 levels) in <top (required)>' # ./test/support/helper/spec/shared_context/homebrew_cask.rb:53:in `block (2 levels) in <top (required)>' ```

issyl0 · 2024-01-21T01:58:51Z

Bevan just pointed out that before merging this we should fix the problems in the cask-versions tap. Yes. I'll do that in the "morning" if Bevan hasn't had time.

bevanjkay · 2024-01-21T02:31:21Z

There's another edge case that needs to be fixed.
We're only looking back one line at the moment to find a preceding comment to an array item.
We need to look recursively and move all lines of comment, with the relevant array item.

I don't believe this currently affects any of our taps, but we should try to fix this.

Library/Homebrew/rubocops/cask/array_alphabetization.rb

Co-authored-by: Bevan J. Kay <email@bevankay.me>

reitermarkus · 2024-01-21T14:17:25Z

How does this behave with rmdir?

uninstall rmdir: [
                   "/dir",
                   "/dir/nested",
                 ]

should be ordered such that the deepest dir is rmdir'd first.

Not sure if we already sort these correctly in the actual uninstall code.

Library/Homebrew/rubocops/cask/array_alphabetization.rb

issyl0 · 2024-01-21T17:13:57Z

How does this behave with rmdir?

Good question. We have 27 occurrences of rmdir: [ arrays, some of which are in zaps which, as you point out, only have trash stanzas alphabetized. But maybe that's intentional, these should be ordered deepest first so they're not covered by this change (it wasn't in the original issue - but I think scope creep is a big thing here with all these edge cases)?

At a glance it seems like we already handle removing these recursively here and/or here.

issyl0 · 2024-01-21T17:31:00Z

Hmm, I see what you mean about the order of the nested paths in rmdir. Should we therefore exclude rmdir from the fields this cop operates on? (Maybe we can fix this later.)

Let's test what our rmdir code does. I see that Casks/g/gcc-aarch64-embedded.rb, as a random example from Homebrew/homebrew-cask#164914, has:

uninstall pkgutil: "arm-gnu-toolchain-#{pkg_version}-darwin-#{arch}-aarch64-none-elf",
          delete:  "/Applications/ArmGNUToolchain/#{pkg_version}/aarch64-none-elf",
          rmdir:   [
            "/Applications/ArmGNUToolchain/#{pkg_version}",
            "/Applications/ArmGNUToolchain",
          ]

which at present would autocorrect to:

uninstall pkgutil: "arm-gnu-toolchain-#{pkg_version}-darwin-#{arch}-aarch64-none-elf",
          delete:  "/Applications/ArmGNUToolchain/#{pkg_version}/aarch64-none-elf",
          rmdir:   [
            "/Applications/ArmGNUToolchain",
            "/Applications/ArmGNUToolchain/#{pkg_version}",
          ]

Testing this autocorrection on an install and uninstall, let's see if those files are correctly deleted:

==> Removing directories if empty:
/Applications/ArmGNUToolchain/13.2.Rel1
/usr/bin/sudo -E -- /bin/rmdir -- /Applications/ArmGNUToolchain/13.2.Rel1
/Applications/ArmGNUToolchain

So it looks like it does the right thing already, most nested first.

$ ls /Applications/ArmGNUToolchain/13.2.Rel1
ls: /Applications/ArmGNUToolchain/13.2.Rel1: No such file or directory
$ ls /Applications/ArmGNUToolchain
ls: /Applications/ArmGNUToolchain: No such file or directory

issyl0 · 2024-01-21T19:37:17Z

Right, so now we're treating zap and uninstall no different to each other, we're at:

a*-m*: Fix RuboCop Cask/ArrayAlphabetization offenses homebrew-cask#164914 and Fix RuboCop Cask/ArrayAlphabetization offenses homebrew-cask-versions#19031 are still good to be reviewed and merged.
I'll need to do another pass at the fixes for a* to m* casks now we're considering more than just zap trash. But it seems fairly small based on a quick run.
I'll need to do another pass at the fixes for n* to z* casks now we're considering more than just zap trash (there's zap delete).
My preference would be to do the above two things in separate PRs once the existing ones are merged, because they're giant enough already.

- Since `zap` can have more than just `trash`.

issyl0 · 2024-01-22T00:07:12Z

If I could please get a review on this, then I can do another brew style --fix run on Cask taps once this is approved as actually code that we want, to avoid any weirdness if there are final changes. 😬

MikeMcQuaid

Looking good, thanks @issyl0!

issyl0 · 2024-01-24T16:21:53Z

I think the last 🧩 here is Homebrew/homebrew-cask#165188.

MikeMcQuaid reviewed Dec 20, 2023

View reviewed changes

Apparently N is alphabetically before c; downcase the comparison

73b3ace

bevanjkay reviewed Dec 21, 2023

View reviewed changes

Library/Homebrew/rubocops/cask/array_alphabetization.rb Outdated Show resolved Hide resolved

razvanazamfirei mentioned this pull request Dec 21, 2023

rubocop: order uninstall/zap methods #16377

Merged

7 tasks

razvanazamfirei reviewed Dec 21, 2023

View reviewed changes

Library/Homebrew/rubocops/cask/array_alphabetization.rb Outdated Show resolved Hide resolved

Library/Homebrew/rubocops/cask/array_alphabetization.rb Outdated Show resolved Hide resolved

issyl0 and others added 4 commits December 22, 2023 00:19

Ignore zap trash stanzas with interpolation

f4754ba

- Interpolating the version into a path is a common pattern, but the interpolations trip up the alphabetization autocorrect quite spectacularly, so let's ignore them (for now?).

Make the test expect_offense calls consistent with other tests

f34accf

Ignore non-zap trash methods

fb124f9

Check uninstall too, avoiding arrays containing commands

9070f6d

issyl0 added 4 commits January 14, 2024 20:55

Appease RuboCop

6a1cb62

There are more arrays that contain non-alphabetizeable commands

338e30f

Autofix Cask/ArrayAlphabetization offenses in test fixtures

2c9e6e4

Add a failing test for comments moving with the alphabetization

e4b4af4

issyl0 marked this pull request as ready for review January 20, 2024 16:01

bevanjkay mentioned this pull request Jan 21, 2024

Fix RuboCop Cask/ArrayAlphabetization offenses Homebrew/homebrew-cask-versions#19031

Merged

9 tasks

bevanjkay reviewed Jan 21, 2024

View reviewed changes

Library/Homebrew/rubocops/cask/array_alphabetization.rb Outdated Show resolved Hide resolved

issyl0 and others added 2 commits January 21, 2024 12:42

Move multi-line comments with the code they're 'attached' to

458844a

Co-authored-by: Bevan J. Kay <email@bevankay.me>

Empty line after guard clause, gah

d5f9eef

reitermarkus reviewed Jan 21, 2024

View reviewed changes

Library/Homebrew/rubocops/cask/array_alphabetization.rb Outdated Show resolved Hide resolved

issyl0 force-pushed the rubocop-cask-zap-arrays-alphabetical branch from 57757ef to 892b475 Compare January 21, 2024 19:24

Treat zap and uninstall the same

693a27d

- Since `zap` can have more than just `trash`.

issyl0 force-pushed the rubocop-cask-zap-arrays-alphabetical branch from 5a8b557 to 693a27d Compare January 21, 2024 19:48

issyl0 mentioned this pull request Jan 21, 2024

Fix new RuboCop Cask/ArrayAlphabetization offenses Homebrew/homebrew-cask-versions#19037

Merged

9 tasks

MikeMcQuaid requested review from reitermarkus, bevanjkay and razvanazamfirei January 22, 2024 12:28

bevanjkay approved these changes Jan 22, 2024

View reviewed changes

MikeMcQuaid approved these changes Jan 24, 2024

View reviewed changes

issyl0 mentioned this pull request Jan 24, 2024

Fix new RuboCop Cask/ArrayAlphabetization offenses Homebrew/homebrew-cask#165188

Merged

9 tasks

issyl0 merged commit a6b8a79 into Homebrew:master Jan 24, 2024
24 checks passed

issyl0 deleted the rubocop-cask-zap-arrays-alphabetical branch January 25, 2024 11:11

github-actions bot added the outdated PR was locked due to age label Mar 2, 2024

github-actions bot locked as resolved and limited conversation to collaborators Mar 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a new RuboCop for alphabetizing `zap trash` array elements #16365

Add a new RuboCop for alphabetizing `zap trash` array elements #16365

issyl0 commented Dec 19, 2023 •

edited

MikeMcQuaid left a comment

p-linnane commented Dec 20, 2023

p-linnane commented Dec 20, 2023

krehel commented Dec 20, 2023

bevanjkay commented Dec 22, 2023 •

edited

bevanjkay commented Jan 1, 2024 •

edited

issyl0 commented Jan 1, 2024

issyl0 commented Jan 1, 2024

bevanjkay commented Jan 1, 2024 •

edited

razvanazamfirei commented Jan 2, 2024

issyl0 commented Jan 2, 2024 •

edited

issyl0 commented Jan 12, 2024

cho-m commented Jan 16, 2024

bevanjkay commented Jan 17, 2024

cho-m commented Jan 17, 2024

bevanjkay commented Jan 17, 2024

issyl0 commented Jan 20, 2024

issyl0 commented Jan 21, 2024

bevanjkay commented Jan 21, 2024 •

edited

reitermarkus commented Jan 21, 2024

issyl0 commented Jan 21, 2024 •

edited

issyl0 commented Jan 21, 2024 •

edited

issyl0 commented Jan 21, 2024

issyl0 commented Jan 22, 2024 •

edited

MikeMcQuaid left a comment

issyl0 commented Jan 24, 2024

Add a new RuboCop for alphabetizing zap trash array elements #16365

Add a new RuboCop for alphabetizing zap trash array elements #16365

Conversation

issyl0 commented Dec 19, 2023 • edited

MikeMcQuaid left a comment

Choose a reason for hiding this comment

p-linnane commented Dec 20, 2023

p-linnane commented Dec 20, 2023

krehel commented Dec 20, 2023

bevanjkay commented Dec 22, 2023 • edited

bevanjkay commented Jan 1, 2024 • edited

issyl0 commented Jan 1, 2024

issyl0 commented Jan 1, 2024

bevanjkay commented Jan 1, 2024 • edited

razvanazamfirei commented Jan 2, 2024

issyl0 commented Jan 2, 2024 • edited

issyl0 commented Jan 12, 2024

cho-m commented Jan 16, 2024

bevanjkay commented Jan 17, 2024

cho-m commented Jan 17, 2024

bevanjkay commented Jan 17, 2024

issyl0 commented Jan 20, 2024

issyl0 commented Jan 21, 2024

bevanjkay commented Jan 21, 2024 • edited

reitermarkus commented Jan 21, 2024

issyl0 commented Jan 21, 2024 • edited

issyl0 commented Jan 21, 2024 • edited

issyl0 commented Jan 21, 2024

issyl0 commented Jan 22, 2024 • edited

MikeMcQuaid left a comment

Choose a reason for hiding this comment

issyl0 commented Jan 24, 2024

Add a new RuboCop for alphabetizing `zap trash` array elements #16365

Add a new RuboCop for alphabetizing `zap trash` array elements #16365

issyl0 commented Dec 19, 2023 •

edited

bevanjkay commented Dec 22, 2023 •

edited

bevanjkay commented Jan 1, 2024 •

edited

bevanjkay commented Jan 1, 2024 •

edited

issyl0 commented Jan 2, 2024 •

edited

bevanjkay commented Jan 21, 2024 •

edited

issyl0 commented Jan 21, 2024 •

edited

issyl0 commented Jan 21, 2024 •

edited

issyl0 commented Jan 22, 2024 •

edited