-
-
Notifications
You must be signed in to change notification settings - Fork 102
Move the directory maker into RSpec::Support #74
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
module RSpec::Support | ||
# @api private | ||
# | ||
# Replacement for fileutils#mkdir_p because we don't want to require parts | ||
# of stdlib in RSpec. | ||
class DirectoryMaker | ||
# @api private | ||
# | ||
# Implements nested directory construction | ||
def self.mkdir_p(path) | ||
stack = path.start_with?(File::SEPARATOR) ? File::SEPARATOR : "." | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This still feels *nix-ish only. I don't have a windows system to check on to see if this works or not. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It'd be good to check that, pretty certain |
||
path.split(File::SEPARATOR).each do |part| | ||
stack = File.join(stack, part) | ||
|
||
begin | ||
unless directory_exists?(stack) | ||
Dir.mkdir(stack) | ||
end | ||
rescue Errno::ENOTDIR | ||
raise Errno::EEXIST.new($!.message) | ||
end | ||
|
||
end | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 This looks much cleaner ( ゚ヮ゚) Any reason for the multi-line There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @cupakromer will prevent churn in future if anything else needs to happen in that conditional. Makes diffs easier to read IMO. |
||
|
||
private | ||
|
||
def self.directory_exists?(dirname) | ||
File.exist?(dirname) && File.directory?(dirname) | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
require 'tmpdir' | ||
|
||
RSpec.shared_context "isolated directory", :isolated_directory => true do | ||
around do |ex| | ||
Dir.mktmpdir do |tmp_dir| | ||
Dir.chdir(tmp_dir, &ex) | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
require "spec_helper" | ||
require "fileutils" | ||
|
||
RSpec::Support.require_rspec_support("directory_maker") | ||
|
||
module RSpec::Support | ||
describe DirectoryMaker do | ||
shared_examples_for "an mkdir_p implementation" do | ||
include_context "isolated directory" | ||
|
||
let(:dirname) { File.join(%w[tmp a recursive structure]) } | ||
|
||
def directory_exists?(dirname) | ||
File.exist?(dirname) && File.directory?(dirname) | ||
end | ||
|
||
it "makes directories recursively" do | ||
mkdir_p.call(dirname) | ||
expect(directory_exists?(dirname)).to be true | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is completely a personal style suggestion. Since the other spec is using the block form and this spec makes the assumption the directory didn't exist to start with, we can change to: expect{
DirectoryMaker.mkdir_p(dirname)
}.to change{ File.exist?(dirname) }.to true There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I think that's better, do you mind if I don't change it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep. Like I said it's more personal style. 😸 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like it this way FWIW There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs to use |
||
|
||
it "does not raise if the directory already exists" do | ||
Dir.mkdir("tmp") | ||
mkdir_p.call(dirname) | ||
expect(directory_exists?(dirname)).to be true | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer this to be a positive expectations: Dir.mkdir("tmp")
DirectoryMaker.mkdir_p(dirname)
expect(Dir.exist?(dirname)).to be true This verifies we actually create the remaining directories. Additionally, if an error is raised the spec will still fail. |
||
|
||
context "when a file already exists" do | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Files always exist. Should this be "when the first part(s) of the path resolve to an existing file"? |
||
before { File.open("tmp", "w") } | ||
|
||
it "raises, as it can't make the directory" do | ||
expect { | ||
mkdir_p.call(dirname) | ||
}.to raise_error(Errno::EEXIST) | ||
end | ||
end | ||
|
||
context "when the path specified is absolute" do | ||
let(:dirname) { "bees/ponies" } | ||
|
||
it "makes directories recursively" do | ||
mkdir_p.call(File.expand_path(dirname)) | ||
expect(directory_exists?(dirname)).to be true | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needs a |
||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With the isolated directory helper changes, I think all specs are now based on relative paths. :-/ per |
||
end | ||
|
||
describe ".mkdir_p" do | ||
subject(:mkdir_p) { DirectoryMaker.method(:mkdir_p) } | ||
it_behaves_like "an mkdir_p implementation" | ||
end | ||
|
||
describe "FileUtils.mkdir_p" do | ||
subject(:mkdir_p) { FileUtils.method(:mkdir_p) } | ||
it_behaves_like "an mkdir_p implementation" | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd suggest add the spec for the cases:
|
||
end | ||
end |
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.
Just noticed this was a class. How about making it a
module
and the method amodule_function
?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.
Is there any reason why that's better? IMO leaving this as a class method is probably the simplest and most obvious thing to do.
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.
FileUtils
is a module. It allows someone to mix it in if they don't want to call the top-level all the time.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.
I think that usecase exists because FileUtils is a collection of library methods. On the other hand this object just encapsulates a single method and is RSpec internal use only. I'm not convinced I think that feature is necessary for this functionality.
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.
@myronmarston @xaviershay @JonRowe any thoughts here?
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.
meh, it's private so I don't particularly care either way.