Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
UdpClient with span support #53429
UdpClient with span support #53429
Changes from all commits
f05bfbf
e5cd921
32050da
d9c5e2b
d8e144c
05b0cf2
14465d0
0ca0fee
a368fff
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
According to a new rule, we need to add triple-slash docs for all new public methods right in product code PR-s. They will be fed to a tool to generate API docs.
You can find examples in
Socket.cs
. My recommendation is to copy and alter documentation text from existing overloadsThere 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.
Added xml doc for new APIs.
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.
This is makes the bar really unfair for community contributors IMO, but I think we should also copy the Remarks sections when present:
https://github.com/dotnet/dotnet-api-docs/blob/f6609e53f05d5c00724aaaf5a31120aa5014b79b/xml/System.Net.Sockets/UdpClient.xml#L2324-L2338
(This applies to all new overloads.)
@carlossanlop any plans to make this process simpler and the code less bloated with long copy-pasted blocks? (Not sure if you are the right person to tag about this.)
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.
The long-term plan is to merge the existing external docs back in to the code, so that the code is the one source of truth.
I don't know when/how this will happen; @carlossanlop is the right person to talk to.
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.
Since this is an async overload, we should not use the blocking
Dns.GetHostAddresses
when resolvinghostname
. We need an async variant ofGetEndpoint
.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 agree we should have an async version of GetEndpoint. That said, the existing SendAsync above just uses sync GetEndpoint. So since we don't have this today, I think we could live without it for this PR and file a separate issue.