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
Add a new RuboCop for alphabetizing zap trash
array elements
#16365
Add a new RuboCop for alphabetizing zap trash
array elements
#16365
Conversation
- 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.
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.
Approach looks good so far!
Very excited to see this, as it's something we often have to fix manually on people's PR's. Great job @issyl0! |
cc @bevanjkay @krehel @miccal @razvanazamfirei as they often deal with this issue as well. |
LOVE. Been on my mind for a while. Awesome to see this @issyl0! |
- 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>
@issyl0 I think it is okay and probably preferred to run this on all the arrays within the We also need to skip |
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? |
I've not forgotten about this, I just had a well-earned festive break. 😁 Back to it this week! |
@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. |
Thanks for the feedback, all! This now scans
|
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. |
What about casks like 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. |
@cho-m How many examples split the |
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:
|
@cho-m For the last one, if the comment is necessary it could go in-line, if we can get the comments to stay. |
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 |
``` 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)>' ```
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. |
There's another edge case that needs to be fixed. I don't believe this currently affects any of our taps, but we should try to fix this. |
Co-authored-by: Bevan J. Kay <email@bevankay.me>
How does this behave with uninstall rmdir: [
"/dir",
"/dir/nested",
] should be ordered such that the deepest dir is Not sure if we already sort these correctly in the actual uninstall code. |
Good question. We have 27 occurrences of At a glance it seems like we already handle removing these recursively here and/or here. |
Hmm, I see what you mean about the order of the nested paths in Let's test what our 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
So it looks like it does the right thing already, most nested first.
|
57757ef
to
892b475
Compare
Right, so now we're treating
|
- Since `zap` can have more than just `trash`.
5a8b557
to
693a27d
Compare
If I could please get a review on this, then I can do another |
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.
Looking good, thanks @issyl0!
I think the last 🧩 here is Homebrew/homebrew-cask#165188. |
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?zap
stanza's trash array alphabetically #16323.zap trash
path is not in[]
if it only contains a single element.