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

draft: use testably abstractions interface #901

Conversation

vbreuss
Copy link
Member

@vbreuss vbreuss commented Oct 28, 2022

@fgreinacher
As discussed in #883 I tried myself at a combination of the two interfaces. Here is a draft with a compiling System.IO.Abstractions library that uses the interfaces from Testably.Abstractions.Interface.

This PR is only intended to discuss the general approach and also to explore other alternatives.

I had some difficulties, which I highlighted in the last commit with corresponding "TODO" code comments:

  • You don't have an explicit type implementing just IFileSystemInfo, but this would be required for implementing ResolveLinkTarget
  • You have overloads for WriteAllLinesAsync with an array instead of IEnumerable<string>. Where does this overload come from?
  • Currently I don't explicitly target .NET5. The MoveTo method with override parameter is available starting with .NET5, but not with netstandard2.1 (which is used in the background). Do you plan to remove explicit support for .NET5 (as it is no longer supported by Microsoft)?
  • Currently the explicit cast in the FileSystemWatcherWrapper can't be implemented, as you don't have a reference to the IFileSystem in the static method. Is this really required?
    Note: I have a Wrap method on the IFileSystemWatcherFactory interface for this scenario...

Changes in the Testably project are tracked by Issue 158.

@fgreinacher
Copy link
Contributor

Many thanks for the initiative here @vbreuss, really appreciated!

I'm out traveling for the long weekend, will have a look mid next week!

@fgreinacher
Copy link
Contributor

Don't get me wrong here, but I'd really like this library to stay independent. I'm totally open to adapt the interfaces to be more compatible downstream, but I'm not eager to take a hard dependency here.

That being said here are my thoughts regarding your questions:

  • You don't have an explicit type implementing just IFileSystemInfo, but this would be required for implementing ResolveLinkTarget

(Not really relevant given my comment above, but we could add that implementation)

  • You have overloads for WriteAllLinesAsync with an array instead of IEnumerable<string>. Where does this overload come from?

Good question, it has been there for a while 😄 If it hurts we could get rid of it, overload resolution should pick the IEnumerable<> overload automatically.

  • Currently I don't explicitly target .NET5. The MoveTo method with override parameter is available starting with .NET5, but not with netstandard2.1 (which is used in the background). Do you plan to remove explicit support for .NET5 (as it is no longer supported by Microsoft)?

No plans to remove it right now, maybe when .NET 7 is released.

  • Currently the explicit cast in the FileSystemWatcherWrapper can't be implemented, as you don't have a reference to the IFileSystem in the static method. Is this really required?
    Note: I have a Wrap method on the IFileSystemWatcherFactory interface for this scenario...

We explicitly highlight that in the readme so this could hurt folks and I think we should not remove it.

@vbreuss
Copy link
Member Author

vbreuss commented Nov 3, 2022

@fgreinacher

Don't get me wrong here, but I'd really like this library to stay independent. I'm totally open to adapt the interfaces to be more compatible downstream, but I'm not eager to take a hard dependency here.

All good!
My PR was not aimed to introduce any dependency, but rather to check and highlight the existing differences in the interfaces!
My bad, if I didn't communicate the intention clear enough 😄

That being said here are my thoughts regarding your questions:

  • You don't have an explicit type implementing just IFileSystemInfo, but this would be required for implementing ResolveLinkTarget

(Not really relevant given my comment above, but we could add that implementation)

  • You have overloads for WriteAllLinesAsync with an array instead of IEnumerable<string>. Where does this overload come from?

Good question, it has been there for a while 😄 If it hurts we could get rid of it, overload resolution should pick the IEnumerable<> overload automatically.

  • Currently I don't explicitly target .NET5. The MoveTo method with override parameter is available starting with .NET5, but not with netstandard2.1 (which is used in the background). Do you plan to remove explicit support for .NET5 (as it is no longer supported by Microsoft)?

No plans to remove it right now, maybe when .NET 7 is released.

  • Currently the explicit cast in the FileSystemWatcherWrapper can't be implemented, as you don't have a reference to the IFileSystem in the static method. Is this really required?
    Note: I have a Wrap method on the IFileSystemWatcherFactory interface for this scenario...

We explicitly highlight that in the readme so this could hurt folks and I think we should not remove it.

I will work on a pull request to implement your suggestion in #883

  • Move out the Interfaces in System.IO.Abstractions into a separate project "TestableIO.System.IO.Abstractions"
  • Move the Wrapper implementation to a project "TestableIO.System.IO.Abstractions.Wrappers"
    I will use this abstractions project as a reference in my interface project to replace the file-related interfaces and keep the ITimeSystem there.
    There will definitely be some changes, but I will highlight them in the PR and we can adress them one by one...

@vbreuss vbreuss closed this Nov 18, 2022
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.

2 participants