feat: Add LinkTarget to FileSystemInfo for .NET 6 targets
#790
Conversation
BrianMcBrayer
changed the title
LinkTarget to FileSystemInfo for .NET 6 targetsLinkTarget to FileSystemInfo for .NET 6 targets
|
I think that this is probably a minor version bump, since it adds to the API without changing existing APIs? But I'm not sure. |
fgreinacher
left a comment
fgreinacher
left a comment
There was a problem hiding this comment.
Thanks a lot @BrianMcBrayer! Left some comments inline
src/System.IO.Abstractions.TestingHelpers/System.IO.Abstractions.TestingHelpers.csproj
Show resolved
Hide resolved
tests/System.IO.Abstractions.TestingHelpers.Tests/MockDirectoryArgumentPathTests.cs
Show resolved
Hide resolved
|
Thank you! I'll try to update it all today |
BrianMcBrayer
left a comment
BrianMcBrayer
left a comment
There was a problem hiding this comment.
I think I addressed all the comments
|
Note that this PR is pending #791 because of the enhancements that will add to |
fgreinacher
left a comment
fgreinacher
left a comment
There was a problem hiding this comment.
Looking very good @BrianMcBrayer 👍
Happy to merge after you rebased/adapted the Mock implementation!
…st snapshots as well. Added mock stubs, but we need to discuss what a better mock implementation would look like.
BrianMcBrayer
force-pushed
the
bug/add-link-target-file-system-info
branch
from
2ffb094 to
467a1c4
Compare
|
@fgreinacher it should be all updated to the latest mocking standards. Let me know if there are any additional changes needed though! Otherwise, I think it's ready to merge after checks pass. |
There was a problem hiding this comment.
@BrianMcBrayer Thanks again, I left some minor suggestions. Please also bump the minor version in https://github.com/BrianMcBrayer/System.IO.Abstractions/blob/bug/add-link-target-file-system-info/version.json#L3 to indicate the new feature.
Co-authored-by: Florian Greinacher <[email protected]>
Co-authored-by: Florian Greinacher <[email protected]>
Co-authored-by: Florian Greinacher <[email protected]>
Co-authored-by: Florian Greinacher <[email protected]>
|
@fgreinacher I accepted your review comments. Can't believe I didn't indent those things correctly. I think the conditional comments threw me off, which is silly. |
fgreinacher
left a comment
fgreinacher
left a comment
There was a problem hiding this comment.
Thanks a ton @BrianMcBrayer and no worries about the minor style issues.
Please bump the minor version in https://github.com/BrianMcBrayer/System.IO.Abstractions/blob/bug/add-link-target-file-system-info/version.json#L3 to indicate the new feature. I would have done this myself but cannot commit to your branch (you can allow that by ticking the "Allow edits and access to secrets by maintainers" checkbox on the right).
fgreinacher
left a comment
fgreinacher
left a comment
There was a problem hiding this comment.
Looking great now, thanks for addressing everything so quickly!
|
This is addressed in release v16.1.1. |
2 participants
Overview
In .NET 6, additional symlinks support was added to
FileSystemInfo. This PR adds at least one of those properties here, theLinkTargetproperty.See #789
We need to discuss the mock portion of this though as I'm not sure how this codebase expects that.