uutils-coreutils-lean@0.10.0: Improve pre_install script - #8393
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 4 reviews per rolling hour; 2 remain after this review. Summary by CodeRabbit
WalkthroughThe ChangesCoreutils installation cleanup
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5371b851-e1e5-48ff-9c6d-791792d69318
📒 Files selected for processing (1)
bucket/uutils-coreutils-lean.json
Included review availability: Your plan includes up to 4 reviews per rolling hour; 3 remain after this review.
|
/verify |
|
All changes look good. Wait for review from human collaborators. uutils-coreutils-lean
|
Relates to #8363
I'd included an install script in my last PR that the reviewer must've decided was too elaborate and it was trimmed down to:
But I have some issues with this. The main one being, that if for any reason
$dirshould ever be$null(like if there's ever a bug in a future Scoop update), this would (instead of what it was meant to do) silently delete any exe file not namedcoreutils.exein the current working directory, which could be disasterous.Furthermore, should
[or]be in the path for any reason, those would be interpreted as wildcard characters and the command would fail.It's also redundant, if you're going to use a wildcard for
-Pathanyway, you might as well use doGet-ChildItem "$dir\*.exe"and skip-Include/-Filteraltogether.Also,
-Filteris considered superior to-Includein the official documentation: "Filters are more efficient than other parameters. The provider applies the filter when the cmdlet gets the objects rather than having PowerShell filter the objects after they're retrieved." (The caveat is that-Filtercan only be used for the FileSystem provider, which is why it doesn't fully supersede-Include.)This is the new line that prevents strange edge-cases and is more performant:
Get-ChildItem -LiteralPath $dir -Filter *.exe | Where-Object Name -ne 'coreutils.exe' | Remove-Itemcoderabbitai suggested I add the
-Fileparameter, and while I was at it, I also single-quoted*.exefor consistency's sake.I understand I'm being picky, but I'm hoping that Scoop eventually defaults to always using the much safer
-LiteralPathparameter for its operations.<manifest-name[@version]|chore>: <general summary of the pull request>