#351: Add IFileSystem property to base classes #352
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This allows people to get at the underlying IFileSystem implementation when developing extension methods. For example, it may make sense to put your extension method on DirectoryBase, but you may require FileBase to actually implement it. This makes that possible. In addition it fixes a bug in MockFileInfo.Replace where the returned FileInfo was for the concrete FileSystem not the mock one (bug caused by implicit casting). I think this commit actually removes the need for supporting implicit casting at all.
|
This implements #351. |
This was referenced Aug 27, 2018
herebebeasties
commented
Aug 27, 2018
| @@ -62,5 +62,7 @@ public interface IMockFileDataAccessor | |||
| IDriveInfoFactory DriveInfo { get; } | |||
|
|
|||
| PathVerifier PathVerifier { get; } | |||
|
|
|||
| IFileSystem FileSystem { get; } | |||
There was a problem hiding this comment.
This feels slightly icky to me, but it was the least invasive change to make, as the various constructors for all the mock classes can then remain the same.
| @@ -18,7 +18,7 @@ public class MockDirectory : DirectoryBase | |||
|
|
|||
| private string currentDirectory; | |||
|
|
|||
| public MockDirectory(IMockFileDataAccessor mockFileDataAccessor, FileBase fileBase, string currentDirectory) | |||
| public MockDirectory(IMockFileDataAccessor mockFileDataAccessor, FileBase fileBase, string currentDirectory) : base(mockFileDataAccessor?.FileSystem) | |||
There was a problem hiding this comment.
The reason the above feels a bit icky is that you end up with these null checks which don't make all that much sense. The alternative here is obviously to pass in both the IMockFileDataAccessor and the IFileSystem to all these. Would people prefer we do that instead?
fgreinacher
reviewed
Sep 5, 2018
fgreinacher
reviewed
Sep 5, 2018
fgreinacher
reviewed
Sep 5, 2018
fgreinacher
reviewed
Sep 5, 2018
fgreinacher
reviewed
Sep 5, 2018
fgreinacher
reviewed
Sep 5, 2018
I really can't see the point of any of this - none of these constructors do any real work.
|
I've addressed all your comments, Florian. Thanks. Merged up to master, tests are all green. |
|
Great, thanks again! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This allows people to get at the underlying IFileSystem implementation when developing extension methods.
For example, it may make sense to put your extension method on DirectoryBase, but you may require FileBase to actually implement it. This makes that possible.
In addition it fixes a bug in MockFileInfo.Replace where the returned FileInfo was for the concrete FileSystem not the mock one (bug caused by implicit casting).
I think this commit actually removes the need for supporting implicit casting at all.