Skip to content

Conversation

@wexman
Copy link
Contributor

@wexman wexman commented Mar 6, 2019

I have added the IFileSystemWatcher, IFile and IDirectory interface for easier mocking...

fgreinacher and others added 9 commits December 7, 2018 10:34
- Do not throw exception when calling DirectoryInfoWrapper.Parent for the root directory (TestableIO#430) by @wexman
- Fix MockDirectoryInfo GetFiles for UNC paths caused by bug in StringExtensions.NormalizeSlashes (TestableIO#422) by @DeveloperGuo
- Pass IFileSystem into FileWrapper instead of FileSystem to allow replacement file systems to be used (TestableIO#432) by @kirbatious
- Make sure FileNotFound exceptions contain path and proper message (TestableIO#427) by @fgreinacher
- Do not delete directories that start with the same path (TestableIO#433) by @updateaman
@wexman wexman changed the title Added IFileSystemWatcher Added IFileSystemWatcher, IFile and IDirectory interfaces Mar 7, 2019
@fgreinacher
Copy link
Contributor

This would implement #175, though I'm not sure if a breaking API change like that would produce more negative than positive echo.

@jpreese @robkeim What's your opinion?

@wexman
Copy link
Contributor Author

wexman commented Mar 12, 2019

That might indeed be a breaking change, but a rather theoretical one, meaning it won't actually break because the interfaces have the same members as the abstract classes...

DirectoryInfoBase CreateDirectory(string path, DirectorySecurity directorySecurity);
string[] GetLogicalDrives();
#endif

Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: extra newline here

@robkeim
Copy link
Contributor

robkeim commented Mar 13, 2019

I also agree with the direction that #175 wants to go, how difficult do you think the breaking changes would be for people consuming the library to fix in their solutions @fgreinacher?

If you're using the var keyword the change shouldn't be breaking for local variables inside functions:

var fileSystemWatcherFactory = new FileSystemWatcherFactory();
var fileSystemWatcher = fileSystemWatcherFactory.CreateNew(); // No breaking change here changing from FileSystemWatcherBase to IFileSystemWatcher

For instances where there are breaking changes (explicitly typed local variables, function parameters, function return type, etc, is a simple replace of XyzBase to IXyz sufficient for all the types that introduced the interface? That's definitely a naive approach, but I can't think of situations off the top of my head where doing so wouldn't be sufficient or would introduce other issues.

If that's the case then I don't think we should be too worried about this breaking change.

@wexman
Copy link
Contributor Author

wexman commented Mar 15, 2019

BTW, do you have any idea why the comments from my previous pull request show up here? Did i do something wrong?

@fgreinacher
Copy link
Contributor

BTW, do you have any idea why the comments from my previous pull request show up here? Did i do something wrong?

(I assume comments == commits) That's just happening because we are updating your branch from master. It's fine :)

@fgreinacher
Copy link
Contributor

OK - let's do this. A couple of things that I'd ask you to do:

  1. Update the version in version.json to 4.0 (as this is a breaking change)
  2. Add <inheritdoc>comments to the new interfaces (just mirror what's done in the implementations) so that we get proper XML docs.
  3. Think about whether it makes sense to add a short chapter to README.md that describes the new use case this change enables

@wexman
Copy link
Contributor Author

wexman commented Mar 18, 2019

Oh, ok. But if we're going that route anyway, maybe we should do the same for all other cases as well? For example, IPath instead of PathBase, IFileInfo instead of FileInfoBase, IDirectoryInfo instead of DirectoryInfoBase and so on?

@robkeim
Copy link
Contributor

robkeim commented Mar 19, 2019

@wexman Yes I think it definitely makes sense to have a consistent API

@wexman
Copy link
Contributor Author

wexman commented Mar 19, 2019

Ok, it's done...

@fgreinacher fgreinacher reopened this Mar 24, 2019
@fgreinacher fgreinacher merged commit 980a57a into TestableIO:master Mar 24, 2019
@fgreinacher
Copy link
Contributor

Merged with some adaptions. Thanks a lot for your work @wexman!

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