Skip to content
This repository has been archived by the owner on Apr 14, 2021. It is now read-only.

Add --dry-run flag to clean #2237

Merged
merged 2 commits into from
Jan 9, 2013
Merged

Add --dry-run flag to clean #2237

merged 2 commits into from
Jan 9, 2013

Conversation

wfarr
Copy link
Contributor

@wfarr wfarr commented Jan 7, 2013

Prints out gems instead of removing them.

@wfarr
Copy link
Contributor Author

wfarr commented Jan 7, 2013

/cc #2236

@ghost
Copy link

ghost commented Jan 8, 2013

The --dry-run output should be identical as though the command was actually run IMO. So it should be something like:

  Bundler.ui.info "Removing #{output}"

  unless Bundler.settings[:dry_run]
    FileUtils.rm_rf(gem_dir)
  end
end

Also while running through the stale_gem_files, stale_gemspec_files and stale_git_cache_dirs you don't need to add a check for dry run setting. You can just surround the existing code in an if/unless block like so:

unless Bundler.settings[:dry_run]
  stale_gem_files.each {|file| FileUtils.rm(file) if File.exists?(file) }
  stale_gemspec_files.each {|file| FileUtils.rm(file) if File.exists?(file) }
  stale_git_cache_dirs.each {|dir| FileUtils.rm_rf(dir) if File.exists?(dir) }
end

What do you think?

@wfarr
Copy link
Contributor Author

wfarr commented Jan 8, 2013

@Rohit I'm 👍 on DRYing things up a bit with just wrapping all the iterators in the check. I do think there's merit in making the output sufficiently different for a dry run though.

@wfarr
Copy link
Contributor Author

wfarr commented Jan 8, 2013

Also, the failed CI status there looks to have been one of the Travis VMs failing on some SSH key related thing. Triggering another build should make it go green.

@wfarr
Copy link
Contributor Author

wfarr commented Jan 9, 2013

All right, CI is green and the code has been DRYed up a bit.

@ghost
Copy link

ghost commented Jan 9, 2013

@indirect can you take a look at this PR? :)

@indirect
Copy link
Member

indirect commented Jan 9, 2013

I think the way clean works could use some refactoring, but that's outside the scope of this pull. :) Looks good, thanks @wfarr!

indirect added a commit that referenced this pull request Jan 9, 2013
@indirect indirect merged commit a2a3ef2 into rubygems:master Jan 9, 2013
@guyisra
Copy link

guyisra commented Feb 12, 2013

anyone knows why heroku uses dry-run ON as default? it seems ever since this pull was introduced no gems are being deleted...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants