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

refactor: improve and consolidate interfaces #906

Merged
merged 17 commits into from
Nov 14, 2022

Conversation

vbreuss
Copy link
Member

@vbreuss vbreuss commented Nov 5, 2022

Make the following changes for the interfaces:

  1. Enable Nullable reference types
  2. Improve and cleanup XML documentation and sort properties and methods in interfaces alphabetically
  3. Consolidate the naming in the factory methods to always use just New; so instead of new DirectoryInfo(path) you should use IFileSystem.DirectoryInfo.New(path). The previous existing factory methods were kept and marked as Obsolete.
  4. Added a Wrap method to the factory interfaces that can be used instead of a direct cast (which is not possible for interfaces) to create a wrapped interface from a "System.IO" type.
  5. Removed the WriteAllLinesAsync with array overload, as it is not defined in System.IO and overload resolution will automatically use the IEnumerable overload.
  6. Added missing methods: ResolveLinkTarget and CreateAsSymbolicLink
  7. Change the return type for Stream-Methods to a custom defined FileSystemStream.
    Note: This should fix the issues from feat: Add Name to IFile.Create (and others) #793 , right?

Added the following extensibility measures:

  • Add IFileSystem FileSystem property to more classes to enable implementing extension methods.
    This was simplified by adding a base interface IFileSystemExtensionPoint
  • Add a IFileSystemExtensibility Extensibility property on IFileSystemInfo and IFileSystemStream which grants access to the underlying wrapped instance or to a metadata dictionary to store additional information together with the instance.

@vbreuss
Copy link
Member Author

vbreuss commented Nov 5, 2022

@fgreinacher :
I tried to refactor (especially the interfaces). Is this the correct approach that you envisioned?

@vbreuss vbreuss marked this pull request as ready for review November 5, 2022 21:57
@vbreuss
Copy link
Member Author

vbreuss commented Nov 13, 2022

@fgreinacher :
Did you have time to have a look at this PR?
If this is through, I plan to add two further updates, but in separate PRs:

  1. Extract the ACL-related method to extension methods in TestableIO.System.IO.Abstractions.Wrappers
  2. Add support for the new methods introduced in .NET 7

@fgreinacher fgreinacher self-requested a review November 14, 2022 08:37
@fgreinacher
Copy link
Contributor

@fgreinacher : Did you have time to have a look at this PR? If this is through, I plan to add two further updates, but in separate PRs:

  1. Extract the ACL-related method to extension methods in TestableIO.System.IO.Abstractions.Wrappers
  2. Add support for the new methods introduced in .NET 7

Sorry @vbreuss, I'll have a look today!

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.

Thanks a lot @vbreuss, this looks great, I left some suggestions/questions!

@vbreuss
Copy link
Member Author

vbreuss commented Nov 14, 2022

Thanks for the review, @fgreinacher !

I implemented all review findings and answered your questions :-)
Could you please ignore the Codacy-Issue, as I can't remove the partial from IFile (seems like a false positive)?

Are the next planned steps also OK for you?

  1. Extract the ACL-related method to extension methods in TestableIO.System.IO.Abstractions.Wrappers
  2. Add support for the new methods introduced in .NET 7
  3. Re-Implement the "ResolveLinkTarget" functionality

Co-authored-by: Florian Greinacher <florian@greinacher.de>
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.

Thanks again for your hard work here!

The next steps are also fine from my side 👍

@vbreuss
Copy link
Member Author

vbreuss commented Nov 14, 2022

@fgreinacher :
It seems Codacy didn't trigger a new scan.

Pull Request Scan

Is there anything I can do?

@mergify mergify bot merged commit 3e99d7f into TestableIO:main Nov 14, 2022
@fgreinacher
Copy link
Contributor

@fgreinacher : It seems Codacy didn't trigger a new scan.

Pull Request Scan

Is there anything I can do?

All good, sometimes it just takes a bit 🐌

vbreuss added a commit to Testably/Testably.Abstractions that referenced this pull request Nov 15, 2022
- Apply renamings done in System.IO.Abstractions (see [Pull Request
#906](TestableIO/System.IO.Abstractions#906))
- Use `global using Testably.Abstractions.FileSystem` so that it can be
more easily replaced with another namespace
- Add `IFileSystemEntity` to `IFileSystemInfo` 
- Remove nesting from IWaitForChangedResult
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.

None yet

2 participants