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: extract ACL related functionality #913

Merged

Conversation

vbreuss
Copy link
Member

@vbreuss vbreuss commented Nov 15, 2022

Remove the ACL-Features from the interfaces and instead implement them as extension methods implemented in "TestableIO.System.IO.Abstractions.Wrappers".

This removes the dependency on System.IO.FileSystem.AccessControl from the interface project.

This is part of the approach discussed in #883

BREAKING CHANGE: This refactoring removes ACL-related methods from the interface project and replaces them with extension methods in TestableIO.System.IO.Abstractions.Wrappers.

@vbreuss
Copy link
Member Author

vbreuss commented Nov 15, 2022

@fgreinacher :
I finished part 1 of the next PR. Could you please have a look?

@fgreinacher fgreinacher self-requested a review November 15, 2022 14:02
@fgreinacher
Copy link
Contributor

I'll have a deeper look at this by the middle of the week.

@siprbaum Do you want to do another pass?

- Replace IFileSystemExtensibility with IFileSystemAclSupport
@vbreuss
Copy link
Member Author

vbreuss commented Nov 27, 2022

@fgreinacher :
I think it would be good to finish this PR before bumping the major version in #919, as this PR also changes the dependencies...

@fgreinacher
Copy link
Contributor

@fgreinacher : I think it would be good to finish this PR before bumping the major version in #919, as this PR also changes the dependencies...

@vbreuss I'd like to push out a clean v18 first. We can then just bump major again for this change, no problem :)

@vbreuss
Copy link
Member Author

vbreuss commented Nov 28, 2022

@fgreinacher : I think it would be good to finish this PR before bumping the major version in #919, as this PR also changes the dependencies...

@vbreuss I'd like to push out a clean v18 first. We can then just bump major again for this change, no problem :)

@fgreinacher: I merged the latest changes from main including the v18 refactoring.
I am not sure, if the change really needs a major version bump; it just removes a reference, from the interface and replaces it with extension methods. However, if somebody only uses the interface project (which didn't exist previously), the ACL-methods are now missing...

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 for your rework @vbreuss - I like it a lot know.

I have a few small suggestions/questions and then we're good to go 🚀

@fgreinacher
Copy link
Contributor

fgreinacher commented Dec 1, 2022

I am not sure, if the change really needs a major version bump; it just removes a reference, from the interface and replaces it with extension methods. However, if somebody only uses the interface project (which didn't exist previously), the ACL-methods are now missing...

It will break people that are currently mocking the methods on the interface level, so I'd rather bump major to not surprise folks :)

Could you bump version.json to 19.0 and update the description to include a BREAKING CHANGE notice like in #919?

@vbreuss
Copy link
Member Author

vbreuss commented Dec 2, 2022

I am not sure, if the change really needs a major version bump; it just removes a reference, from the interface and replaces it with extension methods. However, if somebody only uses the interface project (which didn't exist previously), the ACL-methods are now missing...

It will break people that are currently mocking the methods on the interface level, so I'd rather bump major to not surprise folks :)

Could you bump version.json to 19.0 and update the description to include a BREAKING CHANGE notice like in #919?

@fgreinacher :
I implemented your review comments and updated version to 19.0!

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.

LGTM, thanks @vbreuss 🎉

@mergify mergify bot merged commit 24d1d07 into TestableIO:main Dec 8, 2022
@github-actions
Copy link

github-actions bot commented Dec 8, 2022

This is addressed in release v19.0.1.

@github-actions github-actions bot added the state: released Issues that are released label Dec 8, 2022
@github-actions
Copy link

This is addressed in release v19.0.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state: released Issues that are released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants