Skip to content

Change the FileSystem ctor to allow overriding/decorating individual interfaces #757

Closed

Description

@Krm1t

Hello

We're using the library in one of out projects and i really like it as i don't have to implement wrappers for File, Directory and so on. Also a functioning mock out of the box is nice.

One thing i am missing however is the ability to decorate the interfaces that the FileSystem class uses/returns.
An example could be that i want to decorate IFile with something like an IFileLoggingDecorator or implement retry functionality via decorator or even caching when reading filecontents.
But because of the closed ctor in the FileSystem class i don't see any way to do this without creating wrappers (for the wrapper) which seems counterproductive.
It should be possible to allow this while still maintaining the default ctor to support backwards compatibility.

What are your thoughts?

Activity

  1. fgreinacher commented on Nov 4, 2021

    @fgreinacher
    Contributor

    Thanks for your input!

    I guess this is the same wish as stated in #750. Can you check if the discussion over there helps?

  2. Krm1t commented on Nov 4, 2021

    @Krm1t
    Author

    Thanks for the quick answer.

    I'm not sure how that issue should help mine.
    It talks about mocks. I'm not interested in mocking anything.
    I want the logic of the FileSystem class to work as it does. I just wanna decorate the individual interfaces with some extra logic. without affecting the core implementation.

    One idea that comes to mind now that i think about it, is to create my own implementation of the FileSystem class. Copy the logic from your FileSystem class ctor but modify it to add decorators to the implementations.
    It's not very pretty but it works.

  3. fgreinacher commented on Nov 4, 2021

    @fgreinacher
    Contributor

    I'm not sure how that issue should help mine.

    Quoting myself from the linked issue I guess mocking libraries could help reduce the amount of code you need to write:

    I don't know exactly how NSubsitute works, but I guess you could somehow create mocks/stubs at the interface level and delegate some mocked method to a MockFileSystem instance? If the framework does not support it you could implement it via the Decorator pattern.

    But I understand that you don't want to use such a library, especially in the product code.

    The implementation of FileSystem is really tiny with very little changes in the recent years.

    So, yes, I think the simplest way would be to copy the implementation and adapt it to your needs.

    Otherwise do you think it would be better to make the properties virtual so that you can create a derived FileSystem and override the properties where you want to introduce wrappers?

  4. Krm1t commented on Nov 4, 2021

    @Krm1t
    Author

    Virtual properties would be one way of doing it.
    You could also add protected setters to the properties, that would also do the trick.

    But if I were to think dependency injection. Then the following would be the best solution (in my opinion):

    public FileSystem(
        IDriveInfoFactory driveInfoFactory,
        IDirectoryInfoFactory directoryInfoFactory,
        ...
        )
    {
      DriveInfo = driveInfoFactory;
      DirectoryInfo = directoryInfoFactory;
      ...
    }
    

    You can still keep the default ctor for backwards compatibility.

    Of course this would require a method to help with the dependency setup and therefore also a choice of DI framework - if you don't wanna make packages for every framework. Although i do believe most 3rd party frameworks have the ability to import from Microsoft DI, I know Autofac does.

    Having said this I should also acknowledge that it would require quite a few changes to the library since FileSystem requires an instance of say IDriveInfoFactory but to make an instance of this type we need an instance of IFileSystem. A chicken and egg problem. It could be fixed by decoupling the factory classes from their parent (IFileSystem) but as i say it would take some changes.

  5. fgreinacher commented on Nov 5, 2021

    @fgreinacher
    Contributor

    In general I also prefer the DI/IoC way, but as you mentioned it would require quite some refactoring.

    Trying to be consistent with other classes in this library I'd prefer making the properties virtual.

    Happy to take an PR with this change, otherwise it'll probably take one or two weeks.

  6. added
    area: coreIssues that address the core abstractions & the wrappers
    state: ready to pickIssues that are ready for being worked on
    type: enhancementIssues that propose new functionality
    on Nov 5, 2021
  7. Krm1t commented on Nov 5, 2021

    @Krm1t
    Author

    I may have the time during the weekend. I'll let you know.

  8. added a commit that references this issue on Dec 22, 2021
    a54a8b3
  9. added a commit that references this issue on Dec 22, 2021
    05486f7
  10. github-actions commented on Dec 22, 2021

    @github-actions

    This is addressed in release v16.0.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area: coreIssues that address the core abstractions & the wrappersstate: ready to pickIssues that are ready for being worked onstate: releasedIssues that are releasedtype: enhancementIssues that propose new functionality

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions