New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: do not move unrelated files with same prefix #665
Conversation
@@ -236,10 +236,12 @@ public void MoveDirectory(string sourcePath, string destPath) | |||
sourcePath = FixPath(sourcePath); | |||
destPath = FixPath(destPath); | |||
|
|||
var sourcePathSequence = sourcePath.Split(new[] {Path.DirectorySeparatorChar}, StringSplitOptions.RemoveEmptyEntries); |
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.
Should this also split on Path.AltDirectorySeparatorChar ?
E.g. the underlying question is, what happens when you do Directory.Move("c:/foo/bar", "c:/baz");
If it is supported on the real filessystem the mockFilessystem should also support it.
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.
FixPath
on the lines above (236 and 237) ensures only Path.DirectorySeparatorChar is in the path (Path.AltDirectorySeparatorChar is replaced).
Should it split on Path.AltDirectorySeparatorChar anyway?
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.
Oh, I wasn't aware of that. In that case, it is not needed.
What threw me off is that I couldn't find any tests which ensure that paths with Path.AltDirectorySeparatorChar are actually handeled correctly, at least I couldn't find any when glancing over the code.
Pls don't take this the wrong way, this is not a request for you to add those tests!
It is more or less a question if those tests for Path.AltDirectorySeparatorChar exist at all, e.g. at a central place or how else it is ensured that this works
tests/System.IO.Abstractions.TestingHelpers.Tests/MockDirectoryTests.cs
Outdated
Show resolved
Hide resolved
tests/System.IO.Abstractions.TestingHelpers.Tests/MockDirectoryTests.cs
Outdated
Show resolved
Hide resolved
tests/System.IO.Abstractions.TestingHelpers.Tests/MockDirectoryTests.cs
Outdated
Show resolved
Hide resolved
Thanks for your contribution! Changes look alright to me at a first look, happy to do a deeper review as soon as tests are passing. |
My attempt to fix #664
I'm not very familiar with expected behavior in unix file systems but I guess it covers those as well?
Apologies if this is prematurely.