-
-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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: Discourage the use of rm_f
and rm_rf
in formulae and casks
#17705
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! Could we do this for rmtree
too?
Worth noting because we do However because we do that we can also replace |
Yes, good point/catch.
I think the existing behaviour of Point taken, though, that in other cases yeh this is a good case where we should just use our own APIs here. |
Yeah at least we have that for FileUtils in formulae. The annoying one is Pretty much all existing |
900c0f9
to
a7a1dbd
Compare
87e90fe
to
2e9053b
Compare
Hmmm, autofixing stuff makes all of the tests fail. 🤔 |
@issyl0 I think this could be scoped just to formulae/casks, if that made things easier? |
In formulae and casks, now:
|
a2ff442
to
d161912
Compare
0f48cb3
to
3a48e0a
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.
Looking good!
There appears to be a lack of consensus on the formula fixes PR as to if this is the right approach. Anyone else got any opinions there? |
FileUtils.rm_rf
rm_f
and rm_rf
in formulae and casks
53c2c85
to
16a759d
Compare
- This cop checks for the use of `FileUtils.rm_rf` and suggests using `FileUtils.rm_r` because we should know if we couldn't delete a thing for some reason, not just force it.
- This [seems to be](https://ruby-doc.org/3.3.4/exts/pathname/Pathname.html#method-i-rmtree) equivalent to `FileUtils#rm_r`, so replace it with that.
- Tidy up the node matchers. Either `FileUtils.rm_rf` or `rm_rf` on a `Pathname` instance or `self`.
25f8a84
to
0872966
Compare
|
- After all the work that went into #17705, we don't want the docs disagreeing with what CI says the style should be.
- After all the work that went into #17705, we don't want the docs disagreeing with what CI says the style should be.
- After all the work that went into Homebrew#17705, we don't want the docs disagreeing with what CI says the style should be.
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?FileUtils.rm_rf
and suggests usingFileUtils.rm_r
because we should know if we couldn't delete a thing for some reason, not just force it.