refactor: use Pathname instead of Dir.glob

lib/cleanup_vendor.rb

@@ -14,71 +14,70 @@ class << self
     def run(dir, opts = {})
       summary = []
 
-      filter(dir, DEFAULTS.merge(opts)) do |f|
-        if opts[:summary]
-          files = File.file?(f) ? [f] : dir_entries(f).grep_v('.').map { |file| File.join(f, file) }
-          summary << files.map { |f| File.stat(File.join(dir, f)) }
-        end
+      filter(dir, DEFAULTS.merge(opts)) do |p|
+        summary << collect_summary(dir, p) if opts[:summary]
 
-        puts "Removing #{f}..." if opts[:dry_run] || opts[:verbose]
-        FileUtils.remove_entry(f) unless opts[:dry_run]
+        puts "Removing #{p}..." if opts[:dry_run] || opts[:verbose]
+        FileUtils.remove_entry(p) unless opts[:dry_run]
       end
 
-      if opts[:summary]
-        all_files = summary.flatten
-        count = all_files.count
-        blocks = all_files.map(&:blocks).sum
-        bytes = all_files.map(&:size).sum
-
-        puts 'Summary:'
-        puts format_summary('Removed files:', count)
-        puts format_summary('Total blocks:', blocks)
-        puts format_summary('Total bytes:', bytes)
-      end
+      print_summary(summary) if opts[:summary]
     end
 
     def filter(dir, opts = {})
       raise Error.new('Not a directory') unless dir && File.directory?(dir)
+      return to_enum(:filter, dir, opts) unless block_given?
 
-      unless block_given?
-        return to_enum(:filter, dir, opts)
-      end
+      extensions, filenames, directories, top_level_directories = get_options(opts)
 
-      extensions = opts[:extensions] || []
-      filenames = opts[:filenames] || []
-      directories = opts[:directories] || []
-      top_level_directories = opts[:top_level_directories] || []
-
-      dir_entries(dir) do |f|
-        basename = File.basename(f)
-
-        if File.file?(f)
-          if filenames.include?(basename) || extensions.include?(File.extname(basename).delete('.'))
-            yield(f)
-          end
-        end
+      dir_entries(dir) do |p|
+        basename = p.basename.to_s
+        next if basename == '.'
 
-        if File.directory?(f)
-          if directories.include?(basename)
-            yield(f)
-          elsif top_level_directories.include?(basename)
-            tld = File.join(dir, File.dirname(f))
-            yield(f) unless dir_entries(tld, '*.gemspec').empty?
-          end
+        if p.file? && (filenames.include?(basename) || extensions.include?(p.extname.delete('.')))
+          yield(p)
+        elsif p.directory? && directories.include?(basename)
+          yield(p)
+        elsif p.directory? && top_level_directories.include?(basename) && p.parent.glob('*.gemspec').any?
+          yield(p)
         end
       end
     end
 
-    def dir_entries(dir, pattern = '**/*', &block)
-      Dir.chdir(dir) do
-        Dir.glob(pattern, File::FNM_DOTMATCH, &block)
+    def get_options(opts)
+      %i[extensions filenames directories top_level_directories].map do |option|
+        opts.fetch(option, [])
       end
     end
+    private :get_options
+
+    def dir_entries(dir, &block)
+      Pathname.new(dir).glob('**/*', File::FNM_DOTMATCH, &block)
+    end
     private :dir_entries
 
     def format_summary(prefix, number)
       "\t#{prefix}\t#{number.to_s.rjust(20)}"
     end
     private :format_summary
+
+    def collect_summary(dir, p)
+      files = p.file? ? [p] : dir_entries(p).reject { |p| p.basename.to_s == '.' }
+      files.map(&:stat)
+    end
+    private :collect_summary
+
+    def print_summary(summary)
+      all_files = summary.flatten
+      count = all_files.count
+      blocks = all_files.map(&:blocks).sum
+      bytes = all_files.map(&:size).sum
+
+      puts 'Summary:'
+      puts format_summary('Removed files:', count)
+      puts format_summary('Total blocks:', blocks)
+      puts format_summary('Total bytes:', bytes)
+    end
+    private :print_summary
   end
 end

spec/cleanup_vendor_spec.rb

@@ -1,24 +1,25 @@
 require 'tempfile'
 
 RSpec.describe CleanupVendor do
+  let(:dir) { Pathname.new(Dir.mktmpdir('cleanup_vendor')) }
+
   it 'has a version number' do
     expect(CleanupVendor::VERSION).not_to be nil
   end
 
   describe '.run' do
-    let(:dir) { Pathname.new(Dir.mktmpdir('cleanup_vendor')) }
-
     let!(:extensions) do
       %w[c cpp gem h hpp java log md mk o rdoc txt].map do |ext|
         file = Tempfile.create(['test', ".#{ext}"], Dir.mktmpdir('kept', dir))
-        Pathname.new(file).relative_path_from(dir).to_s
+        Pathname.new(file)
       end
     end
 
     let!(:files) do
       %w[README Makefile LICENSE].map do |file|
-        FileUtils.touch(File.join(dir, file))
-        file
+        f = File.join(dir, file)
+        FileUtils.touch(f)
+        Pathname.new(f)
       end
     end
 
@@ -27,7 +28,7 @@
         subdir = File.join(dir, d)
         Dir.mkdir(subdir)
         Tempfile.create('test', subdir)
-        d
+        Pathname.new(subdir)
       end
     end
 
@@ -38,11 +39,11 @@
         subdir = File.join(topdir, d)
         Dir.mkdir(subdir)
         Tempfile.create('test', subdir)
-        Pathname.new(subdir).relative_path_from(dir).to_s
+        Pathname.new(subdir)
       end
     end
 
-    let(:all_files) { files + dirs + extensions }
+    let(:all_files_count) { files.count + 2 * dirs.count + extensions.count }
 
     before { FileUtils.touch(File.join(dir, 'kept.gemspec')) }
     after { FileUtils.remove_entry(dir) }
@@ -51,17 +52,15 @@
       it 'should remove all matching entries' do
         described_class.run(dir)
 
-        Dir.chdir(dir) do
-          files = Dir.glob('**/*', File::FNM_DOTMATCH).grep_v('.')
-          expect(files).to all(satisfy { |f| f.start_with?('kept') })
-          expect(files).to include(*non_top_level_directories)
-        end
+        files = dir.glob('**/*', File::FNM_DOTMATCH).reject { |p| p === dir }
+        expect(files).to all(satisfy { |p| p.relative_path_from(dir).to_s.start_with?('kept') })
+        expect(files).to include(*non_top_level_directories)
       end
     end
 
     context 'with summary' do
       it 'should display a nice summary' do
-        expected_count = all_files.count
+        expected_count = all_files_count
         expect { described_class.run(dir, summary: true) }.to output(/Summary:\s+Removed files:\s+#{expected_count}/).to_stdout
       end
     end
@@ -70,13 +69,10 @@
       it 'should keep all the entries' do
         expect { described_class.run(dir, dry_run: true) }.to output(/Removing/).to_stdout
 
-        Dir.chdir(dir) do
-          left_files = Dir.glob('**/*', File::FNM_DOTMATCH)
-
-          expect(left_files).to include(*files)
-          expect(left_files).to include(*dirs)
-          expect(left_files).to include(*extensions)
-        end
+        left_files = dir.glob('**/*', File::FNM_DOTMATCH)
+        expect(left_files).to include(*files)
+        expect(left_files).to include(*dirs)
+        expect(left_files).to include(*extensions)
       end
     end
   end
@@ -90,29 +86,32 @@
       expect { described_class.filter(__FILE__) }.to raise_error(CleanupVendor::Error)
     end
 
-    it 'should return an empty list by default' do
-      expect { described_class.filter('.').to be_empty }
-    end
-
     context 'when called with a real directory' do
-      let(:dir) { File.expand_path('../..', __FILE__) }
+      let!(:rb) { Tempfile.create(['test', '.rb'], dir) }
+      let!(:spec) { Dir.mkdir(File.join(dir, 'spec')) }
+      let!(:tmpfile) { Pathname.new(Tempfile.create('test', dir).path) }
+      let!(:fix_filename) { tmpfile.basename.to_s }
+
+      it 'without options it should return with an empty list' do
+        expect { described_class.filter(dir).to be_empty }
+      end
 
       it { expect { |b| described_class.filter(dir, extensions: %w[rb], &b) }.to yield_control }
 
       it 'should filter for extensions' do
         entries = described_class.filter(dir, extensions: %w[rb])
 
-        expect(entries).to all(satisfy { |f| File.file?(f) && f.end_with?('rb') })
+        expect(entries).to all(satisfy { |p| p.file? && p.to_s.end_with?('rb') })
       end
 
       it 'should filter for directories' do
         entries = described_class.filter(dir, directories: %w[spec])
 
-        expect(entries).to all(satisfy { |f| File.directory?(f) && %w[spec .].include?(File.basename(f)) })
+        expect(entries).to all(satisfy { |p| p.directory? && %w[spec].include?(p.basename.to_s) })
       end
 
       it 'should filter for filenames' do
-        expect { |b| described_class.filter(dir, filenames: %w[cleanup_vendor.gemspec], &b) }.to yield_with_args('cleanup_vendor.gemspec')
+        expect { |b| described_class.filter(dir, filenames: [fix_filename], &b) }.to yield_with_args(tmpfile)
       end
     end
   end