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

Fx handler methods for event UploadProgressChanged #8569

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

softwarepronto
Copy link
Contributor

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)

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.
@softwarepronto softwarepronto requested a review from a team as a code owner October 24, 2022 08:58
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 24, 2022
@ghost
Copy link

ghost commented Oct 24, 2022

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

Issue Details

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)

Author: softwarepronto
Assignees: -
Labels:

area-System.Net, community-contribution

Milestone: -

@opbld33
Copy link

opbld33 commented Oct 24, 2022

Learn Build status updates of commit 25e217e:

✅ Validation status: passed

File Status Preview URL Details
snippets/csharp/System.Net/DownloadDataCompletedEventArgs/Overview/asyncmethods.cs ✅Succeeded View

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:

@rzikm
Copy link
Member

rzikm commented Oct 24, 2022

I am pretty sure Snippet41 and Snippet42 are orphaned but this is obsolete documentation.

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.
@softwarepronto softwarepronto requested a review from a team as a code owner October 24, 2022 18:32
@softwarepronto
Copy link
Contributor Author

I am pretty sure Snippet41 and Snippet42 are orphaned but this is obsolete documentation.

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

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.

@opbld30
Copy link

opbld30 commented Oct 24, 2022

Learn Build status updates of commit 6bb2b1b:

✅ Validation status: passed

File Status Preview URL Details
snippets/csharp/System.Net/DownloadDataCompletedEventArgs/Overview/asyncmethods.cs ✅Succeeded View

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:

@opbld32
Copy link

opbld32 commented Oct 24, 2022

Learn Build status updates of commit 54634de:

✅ Validation status: passed

File Status Preview URL Details
snippets/csharp/System.Net/DownloadDataCompletedEventArgs/Overview/asyncmethods.cs ✅Succeeded View

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>
Copy link
Member

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

Copy link
Contributor Author

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?

Copy link
Member

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

Copy link
Contributor

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?

@rzikm
Copy link
Member

rzikm commented Oct 25, 2022

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.

Yes, consistency across languages would be nice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Net community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants