Skip to content

Make rmdir: recursive#6922

Merged
reitermarkus merged 1 commit intoHomebrew:masterfrom
danielbayley:rmdir
Feb 15, 2020
Merged

Make rmdir: recursive#6922
reitermarkus merged 1 commit intoHomebrew:masterfrom
danielbayley:rmdir

Conversation

@danielbayley
Copy link
Copy Markdown
Contributor

@danielbayley danielbayley commented Jan 10, 2020

  • 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 tests with your changes locally?

Following on from Homebrew/homebrew-cask#67806, and Homebrew/homebrew-cask#73867 (comment)

Comment thread Library/Homebrew/cask/artifact/abstract_uninstall.rb Outdated
Comment thread Library/Homebrew/cask/artifact/abstract_uninstall.rb Outdated
@danielbayley
Copy link
Copy Markdown
Contributor Author

Is there a way to run the brew tests just for uninstall_spec and zap_spec? Maybe I'm doing something wrong… but brew tests --only doesn't seem to work.

@danielbayley danielbayley force-pushed the rmdir branch 4 times, most recently from b65fb3f to 0c184e5 Compare January 14, 2020 02:07
Comment thread Library/Homebrew/cask/artifact/abstract_uninstall.rb Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we calling /bin/rmdir instead of using FileUtils.rmdir?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because we're calling it with sudo.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because we're calling it with sudo.

I did wonder about this in the past… Is there no way of doing sudo with the native Ruby methods?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, do we still need print_stderr: false here, since we now have the unless clause prior?

Comment thread Library/Homebrew/cask/artifact/abstract_uninstall.rb Outdated
Comment thread Library/Homebrew/test/cask/artifact/zap_spec.rb Outdated
Comment thread Library/Homebrew/cask/artifact/abstract_uninstall.rb Outdated
Comment thread Library/Homebrew/cask/artifact/abstract_uninstall.rb Outdated
@reitermarkus
Copy link
Copy Markdown
Member

I think there needs to be a separate function for the recursion, otherwise

ohai "Removing directories if empty:"

will be called multiple times.

@danielbayley
Copy link
Copy Markdown
Contributor Author

danielbayley commented Jan 17, 2020

Ok, pretty sure I've got it now @reitermarkus 👍

Is there a way to run the brew tests just for uninstall_spec and zap_spec? Maybe I'm doing something wrong… but brew tests --only doesn't seem to work.

I still would like to know this…

Comment thread Library/Homebrew/cask/artifact/abstract_uninstall.rb Outdated
Comment thread Library/Homebrew/cask/artifact/abstract_uninstall.rb Outdated
@danielbayley danielbayley force-pushed the rmdir branch 2 times, most recently from a144a1c to 0a49d73 Compare January 21, 2020 06:24
@danielbayley
Copy link
Copy Markdown
Contributor Author

Sorry, I don't quite follow… Does all? not need to operate on an array of already resolved_paths (therefore inside this method call)? Also, the second directories argument can be a [glob pattern] string

@reitermarkus ?

@reitermarkus
Copy link
Copy Markdown
Member

Instead of next false, create a success = true before the each_resolved_path and set that to false and at the end return success.

@danielbayley danielbayley force-pushed the rmdir branch 2 times, most recently from 7984c73 to c992f85 Compare February 3, 2020 09:43
Comment thread Library/Homebrew/cask/artifact/abstract_uninstall.rb Outdated
Comment thread Library/Homebrew/cask/artifact/abstract_uninstall.rb Outdated
@danielbayley danielbayley force-pushed the rmdir branch 2 times, most recently from 8d04e5e to f97a61e Compare February 7, 2020 02:15
Comment thread Library/Homebrew/cask/artifact/abstract_uninstall.rb Outdated
@vitorgalvao
Copy link
Copy Markdown
Contributor

Leaving a note that we’ll need to update the documentation as well.

@danielbayley
Copy link
Copy Markdown
Contributor Author

Leaving a note that we’ll need to update the documentation as well.

@vitorgalvao Do you mean in this PR?

@vitorgalvao
Copy link
Copy Markdown
Contributor

@vitorgalvao Do you mean in this PR?

No, in the cask repo.

@reitermarkus reitermarkus merged commit cb4316d into Homebrew:master Feb 15, 2020
@reitermarkus
Copy link
Copy Markdown
Member

Thanks, @danielbayley!

@lock lock bot added the outdated PR was locked due to age label Mar 17, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Mar 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

outdated PR was locked due to age

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants