Skip to content
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

Make paths case-sensitive on Unix #418

Merged
merged 21 commits into from
Dec 7, 2018
Merged

Make paths case-sensitive on Unix #418

merged 21 commits into from
Dec 7, 2018

Conversation

rkoeninger
Copy link
Contributor

@rkoeninger rkoeninger commented Dec 4, 2018

Fixes #321

  • Added properties bool CaseSensitive, StringComparer Comparer, StringComparison Comparison to MockFileSystem, IMockFileDataAccessor. The other Mock* classes use these properties to perform string comparisons and to opt to do ToUpper/ToLower or not.
  • MockDirectory.GetLogicalDrives() returns drive letters in upper case instead of lower case.
  • Moved HasIllegalCharacters, CheckInvalidPathChars from MockPath to PathVerifier as it's the more appropriate place.
  • Added IsWindowsPlatform() to MockUnixSupport.
  • Removed the optional Func<bool> argument to methods on MockUnixSupport that overrides platform detection. Tests that need to run on a specific platform are labeled with the relevant *Only attribute.
  • Added/updated tests as needed. Tests that specifically asserted case-insensitive behavior were labeled WindowsOnly. Similar tests were added to assert case-sensitive behavior and are labeled UnixOnly.

Fixes #395

  • Added special case handling to MockDirectory.GetParent for Unix to make sure a slash-root parent is properly identified and returned.
  • Added a test for this, labeled UnixOnly.

Fixes #405

  • Fixed MockDirectory.GetFiles to handle un-normalized slashes:
    • Use of alternate slashes.
    • Missing or present trailing slash.
    • Redundant slashes.
  • Added cross platform test for this.

@rkoeninger
Copy link
Contributor Author

rkoeninger commented Dec 4, 2018

@fgreinacher @ericnewton76 thoughts?

There's also a comment in there about a pre-existing thing that I had a question about.

@fgreinacher
Copy link
Contributor

Wow, that’s a lot of improvements, thanks a lot! Please give me one or two days to have a look at this.

Copy link
Contributor

@ericnewton76 ericnewton76 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mine are just nitpicks.

I assume this works correctly, although there's several lines touched that didn't otherwise need be.

Copy link
Contributor

@fgreinacher fgreinacher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look great! I left some nit-picky comments, will merge as soon as those are addressed!

System.IO.Abstractions.TestingHelpers/MockDriveInfo.cs Outdated Show resolved Hide resolved
System.IO.Abstractions.TestingHelpers/MockFile.cs Outdated Show resolved Hide resolved
System.IO.Abstractions.TestingHelpers/MockFile.cs Outdated Show resolved Hide resolved
System.IO.Abstractions.TestingHelpers/PathVerifier.cs Outdated Show resolved Hide resolved
System.IO.Abstractions.TestingHelpers/PathVerifier.cs Outdated Show resolved Hide resolved
@rkoeninger
Copy link
Contributor Author

@fgreinacher Covered everything listed so far. Let me know if you see anything else.

Copy link
Contributor

@fgreinacher fgreinacher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect now!

@fgreinacher fgreinacher merged commit ade59ac into TestableIO:master Dec 7, 2018
@fgreinacher
Copy link
Contributor

Thanks again for all your work on this @rkoeninger 🎉

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

Successfully merging this pull request may close these issues.

3 participants