62606 Return fully qualified path from FileSystemEventArgs #63051

slask · 2021-12-21T18:42:02Z

The FullPath for FileSystemEventArgs and related OldFullPath for RenamedEventArgs now return fully qualified paths as per the documentation and the name of these properties and according to the discussions in the issue below.

Fix #62606

Gets the fully qualified path after combining the directory and name part as per the thread discussion. Also added and updated tests for the new change. Fix dotnet#62606

To be consistent with new behavior of FullPath from FileSystemEventArgs same change was applied for RenamedEventArgs including updated tests. Fix dotnet#62606

ghost · 2021-12-21T18:42:09Z

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

The FullPath for FileSystemEventArgs and related OldFullPath for RenamedEventArgs now return fully qualified paths as per the documentation and the name of these properties and according to the discussions in the issue below.

Fix #62606

Author:	slask
Assignees:	-
Labels:	`area-System.IO`
Milestone:	-

dnfadmin · 2021-12-21T18:42:15Z

All CLA requirements met.

slask · 2021-12-22T04:09:28Z

Hi @jozkee . Based on the feedback from the failed tests I believe the fix is good but the tests are too Windows OS oriented. They were the same before my changes but back then it was not an issue since the FullPath was not actually returning a truly fully qualified path but mostly a simple concatenation of directory and file name.

Two options that I see now are:

remove the windows specific test cases (InlineData attribute instances)
extract the windows specific test cases and mark them somehow to be run on Windows only. I might need some pointers on how to do that. Is it the [ActiveIssue(...)] or [PlatformSpecific(TestPlatforms.Windows)] attribute?

Please let me know your thoughts. Thanks!

In the context of using an actual full path which can vary on Unix vs Windows Fix dotnet#62606

src/libraries/System.IO.FileSystem.Watcher/tests/Args.FileSystemEventArgs.cs

src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemEventArgs.cs

src/libraries/System.IO.FileSystem.Watcher/src/System/IO/RenamedEventArgs.cs

src/libraries/System.IO.FileSystem.Watcher/tests/Args.FileSystemEventArgs.cs

src/libraries/System.IO.FileSystem.Watcher/tests/Args.RenamedEventArgs.cs

src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemEventArgs.cs

jozkee · 2021-12-28T18:54:17Z

@slask thanks for your PR.

I just saw this:

Two options that I see now are:

remove the windows specific test cases (InlineData attribute instances)
extract the windows specific test cases and mark them somehow to be run on Windows only. I might need some pointers on how to do that. Is it the [ActiveIssue(...)] or [PlatformSpecific(TestPlatforms.Windows)] attribute?

Second option is the way to go, which based on what I saw you already did correctly.

Adjusted unit tests to match the new logic. Fix dotnet#62606

The previous buggy implementation of RenamedEventArgs produced a convenient side-effect for PhysicalFilesWatcher. Without the trailing slash adde by RenamedEventArgs the root was longer than the fullPath by a trailing slash. Fix dotnet#62606

src/libraries/System.IO.FileSystem.Watcher/tests/Args.FileSystemEventArgs.cs

Fix dotnet#62606

src/libraries/System.IO.FileSystem.Watcher/tests/Args.FileSystemEventArgs.cs

src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PhysicalFilesWatcher.cs

Implemented most of the code review suggestions. Fix dotnet#62606

…eturn-fully-qualified-path-FileSystemEventArgs

Also made unit tests more focused by removing needless asserts from the tests Fix dotnet#62606

Reverted the changes to PhysicalFilesWatcher and added back the trailing separator behavior as per code review suggestion Fix dotnet#62606

ghost · 2022-01-18T20:16:50Z

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

slask · 2022-01-21T10:19:58Z

Linking doc issue created for breaking change: dotnet/docs#27916

slask · 2022-01-21T10:28:54Z

@jozkee / @ericstj I created the breaking change doc issue and cross-linked them.
I also emailed [email protected] with the docs issue but the mail got rejected.

I don't think I have permission to remove the tag from this PR.

slask · 2022-01-25T06:12:44Z

Hi @jozkee, is there something else I need to do to finalize this PR?

slask · 2022-02-07T04:56:02Z

Hi @jozkee. Friendly reminder to move this PR forward.

jozkee

Just minor nits in test code; otherwise, looks good to me, thanks.

src/libraries/System.IO.FileSystem.Watcher/tests/Args.FileSystemEventArgs.cs

As per code review suggestions. Fix dotnet#62606

Pipeline retrigger. Some pipelines timed out, most probably transient issues. Fix dotnet#62606

slask · 2022-02-07T16:14:40Z

@jozkee I have made the small changes you mentioned to the tests but I see these 7 jobs are timing out for some reason but I don't see how they are related to the changes I made...

jozkee · 2022-02-07T17:33:45Z

@slask the time-outs are most likely unrelated. Just for safety, I hit re-run, let's hope they finish this time.

jozkee · 2022-02-07T21:17:32Z

@slask, thanks for your contribution!

danmoseley · 2022-09-20T20:42:32Z

#68883

slask added 2 commits December 21, 2021 20:07

FullPath property now returns fully qualified path

280021d

Gets the fully qualified path after combining the directory and name part as per the thread discussion. Also added and updated tests for the new change. Fix dotnet#62606

OldFullPath returns fully qualified path

21c655c

To be consistent with new behavior of FullPath from FileSystemEventArgs same change was applied for RenamedEventArgs including updated tests. Fix dotnet#62606

dotnet-issue-labeler bot added the area-System.IO label Dec 21, 2021

ghost added the community-contribution Indicates that the PR has been added by a community member label Dec 21, 2021

danmoseley requested a review from jozkee December 21, 2021 23:10

Marked some test cases as Windows OS specific

fbb4113

In the context of using an actual full path which can vary on Unix vs Windows Fix dotnet#62606

jozkee reviewed Dec 28, 2021

View reviewed changes

slask added 2 commits December 29, 2021 14:37

Using Path.Join instead of the custom Combine

0375648

Adjusted unit tests to match the new logic. Fix dotnet#62606

jozkee reviewed Jan 4, 2022

View reviewed changes

src/libraries/System.IO.FileSystem.Watcher/tests/Args.FileSystemEventArgs.cs Outdated Show resolved Hide resolved

Handle relative paths from CWD in given drive

f28bf24

Fix dotnet#62606

jozkee reviewed Jan 12, 2022

View reviewed changes

slask added 4 commits January 17, 2022 18:46

Clearly separated unix and windows test cases

a025928

Implemented most of the code review suggestions. Fix dotnet#62606

Merge branch 'main' of https://github.com/dotnet/runtime into 62606-r…

5a663e9

…eturn-fully-qualified-path-FileSystemEventArgs

Use EnsureTrailingSeparator instead of custom code

5ff61d5

Also made unit tests more focused by removing needless asserts from the tests Fix dotnet#62606

Keeping the old behavior with trailing separator

aaf4b45

Reverted the changes to PhysicalFilesWatcher and added back the trailing separator behavior as per code review suggestion Fix dotnet#62606

ericstj added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Jan 18, 2022

ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Jan 18, 2022

slask mentioned this pull request Jan 21, 2022

[Breaking change]: FileSystemEventArgs.FullPath returns a truly fully qualified path dotnet/docs#27916

Closed

2 tasks

adamsitnik assigned jozkee Jan 26, 2022

jozkee approved these changes Feb 7, 2022

View reviewed changes

slask added 2 commits February 7, 2022 09:39

Used Path.Combien vs PathInternalEnsureSeparator

646eeac

As per code review suggestions. Fix dotnet#62606

Added blank line for readability

c657af2

Pipeline retrigger. Some pipelines timed out, most probably transient issues. Fix dotnet#62606

jozkee merged commit 2a1b15d into dotnet:main Feb 7, 2022

runfoapp bot mentioned this pull request Feb 8, 2022

System.Net.NetworkInformation.Tests.PingTest.SendPingWithHostAndTimeoutAndBuffer failing on OSX #64963

Closed

adamsitnik mentioned this pull request Mar 7, 2022

remove invalid System.IO.FileSystem.Watcher tests #66296

Merged

ghost locked as resolved and limited conversation to collaborators Apr 1, 2022

jeffhandley removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Apr 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

62606 Return fully qualified path from FileSystemEventArgs #63051

62606 Return fully qualified path from FileSystemEventArgs #63051

slask commented Dec 21, 2021

ghost commented Dec 21, 2021

dnfadmin commented Dec 21, 2021 •

edited

Loading

slask commented Dec 22, 2021 •

edited

Loading

jozkee commented Dec 28, 2021

ghost commented Jan 18, 2022

slask commented Jan 21, 2022

slask commented Jan 21, 2022 •

edited

Loading

slask commented Jan 25, 2022

slask commented Feb 7, 2022

jozkee left a comment

slask commented Feb 7, 2022

jozkee commented Feb 7, 2022

jozkee commented Feb 7, 2022

danmoseley commented Sep 20, 2022

62606 Return fully qualified path from FileSystemEventArgs #63051

62606 Return fully qualified path from FileSystemEventArgs #63051

Conversation

slask commented Dec 21, 2021

ghost commented Dec 21, 2021

dnfadmin commented Dec 21, 2021 • edited Loading

slask commented Dec 22, 2021 • edited Loading

jozkee commented Dec 28, 2021

ghost commented Jan 18, 2022

slask commented Jan 21, 2022

slask commented Jan 21, 2022 • edited Loading

slask commented Jan 25, 2022

slask commented Feb 7, 2022

jozkee left a comment

Choose a reason for hiding this comment

slask commented Feb 7, 2022

jozkee commented Feb 7, 2022

jozkee commented Feb 7, 2022

danmoseley commented Sep 20, 2022

dnfadmin commented Dec 21, 2021 •

edited

Loading

slask commented Dec 22, 2021 •

edited

Loading

slask commented Jan 21, 2022 •

edited

Loading