Update #13 , Correct corner cases #14
Conversation
…does not exist, destination is a directory corner cases. piotrmurach#13
There was a problem hiding this comment.
Thank you for working on this. It's looking good but before I can merge, could you please make a few changes and add tests cases to copy_file_spec.rb that I mentioned in my comments. For the return value, you could possibly slightly modify the existing test cases. I'd also consider adding a test case for copying a file into a directory and handle the case when the source and destination paths are both directories.
| if ::File.directory? dest_path | ||
| dest_path = ::File.join(dest_path, ::File.basename(source_path)) | ||
| end |
There was a problem hiding this comment.
Good catch! However, I think we shouldn't change the destination path. If someone wants to copy a file to an existing directory, we should allow for it and the file collision will handle the situation when the file already exists. For example,
TTY::File.copy_file("foo/bar.rb", "baz/")
# => baz/bar.rbAlso, any changes like this require a test case.
| unless ::File.file? source_path | ||
| log_status(:error, source_path, verbose: verbose, color: :red) | ||
| return | ||
| end |
There was a problem hiding this comment.
I believe this statement makes the directory check unreachable. Also, I much prefer to allow copying a file to a directory location rather than providing an error. Wouldn't you agree?
|
|
||
| copy_metadata(source_path, dest_path, verbose: verbose, noop: noop, | ||
| color: color) | ||
| file_dest |
There was a problem hiding this comment.
We need a test case that confirms this behaviour so that it stays true in future releases.
| @@ -309,10 +319,11 @@ def copy_file(source_path, *args, context: nil, force: false, skip: false, | |||
| content = block[content] if block | |||
| content | |||
| end | |||
| return unless preserve | |||
| return file_dest unless preserve | |||
There was a problem hiding this comment.
We need a test case that confirms this behaviour so that it stays true in future releases.
| if directory? | ||
| notify(:error, :red) | ||
| return | ||
| end |
There was a problem hiding this comment.
I don't believe this is necessary if we allow copying a file to a directory destination. This should never be the case unless someone tries to copy a directory to directory, I guess we could test for that case. This would require another test case.
Describe the change
Why are we doing this?
Benefits
Drawbacks
Requirements
Put an X between brackets on each line if you have done the item:
[] Tests written & passing locally?
[] Code style checked?
[] Rebased with
masterbranch?[] Documentaion updated?