-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Fx handler methods for event UploadProgressChanged #8569
base: main
Are you sure you want to change the base?
Conversation
The documentation for WebClient.UploadProgressChanged event is as follows: https://learn.microsoft.com/en-us/dotnet/api/system.net.webclient.uploadprogresschanged?view=net-6.0 This documentation for UploadProgressChanged uses <Snippet3>that originally included: client.UploadFileCompleted += new UploadFileCompletedEventHandler(UploadFileCallback2); client.UploadProgressChanged += new UploadProgressChangedEventHandler(UploadProgressCallback); The documentation page for UploadProgressChanged then contains a reference to Snippet42 to document the above methods but Snippet42 contains the following methods: UploadProgressCallback DownloadProgressCallback: this is a download method not used in Snippet3 Snippet4 contains the documentation needed by Snippet3 -- the definition of the UploadFileCallback2 method. Snippet4 has no UploadProgressChangedEventHandler. I made a copy of the original UploadProgressCallback and called it UploadProgressCallback2. I placed this in Snippet4 and updated Snippet3 to reference UploadProgressCallback2 instead of UploadProgressCallback. Once asyncmethods.cs is updated, I will change the source to this link: https://learn.microsoft.com/en-us/dotnet/api/system.net.webclient.uploadprogresschanged?view=net-6.0 I will swap out the reference to Snippet42 and replace it with Snippet4. I am pretty sure Snippet41 and Snippet42 are orphaned but this is obsolete documentation. I'd rather fix what is obviously broken (Snippet4) than purge unused code.
Tagging subscribers to this area: @dotnet/ncl Issue DetailsSummaryThe documentation for WebClient.UploadProgressChanged event is as follows: https://learn.microsoft.com/en-us/dotnet/api/system.net.webclient.uploadprogresschanged?view=net-6.0 This documentation for UploadProgressChanged uses that originally included: The documentation page for UploadProgressChanged then contains a reference to Snippet42 to document the above methods but Snippet42 contains the following methods: UploadProgressCallback Snippet4 contains the documentation needed by Snippet3 -- the definition of the UploadFileCallback2 method. Snippet4 has no UploadProgressChangedEventHandler. I made a copy of the original UploadProgressCallback and called it UploadProgressCallback2. I placed this in Snippet4 and updated Snippet3 to reference UploadProgressCallback2 instead of UploadProgressCallback. Once asyncmethods.cs is updated, I will change the source to this link: https://learn.microsoft.com/en-us/dotnet/api/system.net.webclient.uploadprogresschanged?view=net-6.0 I will swap out the reference to Snippet42 and replace it with Snippet4. I am pretty sure Snippet41 and Snippet42 are orphaned but this is obsolete documentation. I'd rather fix what is obviously broken (Snippet4) than purge unused code. Fixes #Issue_Number (if available)
|
Learn Build status updates of commit 25e217e: ✅ Validation status: passed
For more details, please refer to the build report. Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report. For any questions, please:
|
Snippet42 is used by DownloadProgressChangedEvengArgs documentation Since Snippet5 is already used by documentation for UploadFileCompleted (and UploadFileCompletedEventArgs), I don't think we should add the UploadProgressCallback2 to it. It should be a separate snippet to be referenced from UploadProgressChanged documentation |
The documentation for UploadProgressChanged is incorrectly using snippet42. Snippet44 represents the method subscribed to the events in the UploadProgressChanged documentation.
C# only fix -- Snippet4 was related in the documentation to Snippt42. The methods used in Snippet4 did not match Snippet42. Snippet44 was created so that it shows the methods subscribed to the events by Snippet4.
I fixed it for C#. If you agree with the change for C# then I will fix CPP and VB.NET in the same manner. |
Learn Build status updates of commit 6bb2b1b: ✅ Validation status: passed
For more details, please refer to the build report. Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report. For any questions, please:
|
Learn Build status updates of commit 54634de: ✅ Validation status: passed
For more details, please refer to the build report. Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report. For any questions, please:
|
client.UploadFileAsync(uri, "POST", fileName); | ||
Console.WriteLine("File upload started."); | ||
} | ||
//</Snippet4> | ||
|
||
//<Snippet44> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the snippet should not include the UploadFileCallback2 method to be consistent with examples for other event handlers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UploadProgressChanged has two events subscribed.
UploadFileCompleted has two event subscribed.
UploadValuesCompleted has zero events subscribed. This is on my list to fix.
If we want to make it consistent
UploadProgressChanged should have one event (remove one)
UploadFileCompleted should have one event (remove one)
UploadValuesCompleted should have one event (add one)
Is this how we should handle these last three events?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can have multiple events subscribed to, IMO it does not have to show the definition of the other handlers. I.e. show only UploadFileCompleted handler definition in docs for UploadFileCompleted event, as in https://learn.microsoft.com/en-us/dotnet/api/system.net.webclient.uploadfilecompleted?view=net-6.0&branch=main
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@softwarepronto Do you plan to make any more changes here?
Yes, consistency across languages would be nice |
Summary
The documentation for WebClient.UploadProgressChanged event is as follows: https://learn.microsoft.com/en-us/dotnet/api/system.net.webclient.uploadprogresschanged?view=net-6.0
This documentation for UploadProgressChanged uses that originally included:
client.UploadFileCompleted += new UploadFileCompletedEventHandler(UploadFileCallback2);
client.UploadProgressChanged += new UploadProgressChangedEventHandler(UploadProgressCallback);
The documentation page for UploadProgressChanged then contains a reference to Snippet42 to document the above methods but Snippet42 contains the following methods: UploadProgressCallback
DownloadProgressCallback: this is a download method not used in Snippet3
Snippet4 contains the documentation needed by Snippet3 -- the definition of the UploadFileCallback2 method. Snippet4 has no UploadProgressChangedEventHandler. I made a copy of the original UploadProgressCallback and called it UploadProgressCallback2. I placed this in Snippet4 and updated Snippet3 to reference UploadProgressCallback2 instead of UploadProgressCallback.
Once asyncmethods.cs is updated, I will change the source to this link: https://learn.microsoft.com/en-us/dotnet/api/system.net.webclient.uploadprogresschanged?view=net-6.0
I will swap out the reference to Snippet42 and replace it with Snippet4.
I am pretty sure Snippet41 and Snippet42 are orphaned but this is obsolete documentation. I'd rather fix what is obviously broken (Snippet4) than purge unused code.
Fixes #Issue_Number (if available)