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

Port System.Diagnostics source code comments to Docs #2336

Open
wants to merge 15 commits into
base: master
from

Conversation

4 participants
@carlossanlop
Copy link
Member

commented Apr 17, 2019

Summary

Automatically ported triple slash source code comments found in System.Diagnostics that did not exist in dotnet-api-docs.

Fixes #Issue_Number (if available)

@carlossanlop

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

@mairaw @rpetrusha please take a look.

Adding area owners @wtgodbe, @krwq. Please take a look too, the comments had weird wording, I tried to fix as much as possible but I think it still would need some tweaking.

carlossanlop added some commits Apr 24, 2019

Update Activity.xml
Updated a multiline comment based on Will's suggestion.
Apply suggestions from code review
Suggested changes by wtgodbe.
Update Activity.xml
Removed newline.
Update DiagnosticListener.xml
Removed new lines.
Update DiagnosticSource.xml
Newlines and link to Write API.

@mairaw mairaw added the new-content label Apr 26, 2019

@mairaw mairaw added this to In progress in April 2019 via automation Apr 26, 2019

@mairaw

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

I'll start reviewing this one now but the build report has some warnings for this PR.

@rpetrusha
Copy link
Contributor

left a comment

This is only a partial review, @carlossanlop, but I've left a number of comments. Much of the content of the <summary> sections is much too long and is more appropriate in the Remarks section. I'll finish reviewing the PR over the weekend.

Show resolved Hide resolved xml/System.Diagnostics/Activity.xml Outdated
Show resolved Hide resolved xml/System.Diagnostics/Activity.xml Outdated
Show resolved Hide resolved xml/System.Diagnostics/Activity.xml Outdated
Show resolved Hide resolved xml/System.Diagnostics/Activity.xml Outdated
Show resolved Hide resolved xml/System.Diagnostics/Activity.xml Outdated
Show resolved Hide resolved xml/System.Diagnostics/Activity.xml Outdated
Show resolved Hide resolved xml/System.Diagnostics/Activity.xml Outdated
Show resolved Hide resolved xml/System.Diagnostics/Activity.xml
Show resolved Hide resolved xml/System.Diagnostics/DiagnosticListener.xml
Show resolved Hide resolved xml/System.Diagnostics/DiagnosticListener.xml Outdated
@mairaw
Copy link
Contributor

left a comment

Some comments but you probably get the gist of my suggestions. Please make sure I didn't change the meaning of the descriptions.

Show resolved Hide resolved xml/System.Diagnostics/Activity.xml Outdated
Show resolved Hide resolved xml/System.Diagnostics/Activity.xml Outdated
Show resolved Hide resolved xml/System.Diagnostics/Activity.xml Outdated
Show resolved Hide resolved xml/System.Diagnostics/Activity.xml Outdated
Show resolved Hide resolved xml/System.Diagnostics/Activity.xml Outdated
Show resolved Hide resolved xml/System.Diagnostics/Activity.xml Outdated
Show resolved Hide resolved xml/System.Diagnostics/Activity.xml Outdated
Show resolved Hide resolved xml/System.Diagnostics/Activity.xml Outdated
Show resolved Hide resolved xml/System.Diagnostics/Activity.xml Outdated
Show resolved Hide resolved xml/System.Diagnostics/Activity.xml Outdated
@mairaw

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

Apparently, @rpetrusha and I were reviewing the same PR at the same time 😆 let's clean up this a bit and then we can continue with the review on this one

@mairaw

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

I also removed some of the duplicate comments from Ron and I.

carlossanlop and others added some commits Apr 29, 2019

Apply suggestions from code review
Suggestions by rpetrusha and mairaw

Co-Authored-By: carlossanlop <1175054+carlossanlop@users.noreply.github.com>
Update xml/System.Diagnostics/Activity.xml
Co-Authored-By: carlossanlop <1175054+carlossanlop@users.noreply.github.com>
Update xml/System.Diagnostics/Activity.xml
Co-Authored-By: carlossanlop <1175054+carlossanlop@users.noreply.github.com>

carlossanlop and others added some commits Apr 29, 2019

Update xml/System.Diagnostics/Activity.xml
Removing missed summary line
Update Activity.xml
Removing an unexpected "list" tag that was missed in a previous suggestion
Update Activity.xml
Removing unexpected item element.
Apply suggestions from code review
Suggestion I missed by rpetrusha

Co-Authored-By: carlossanlop <1175054+carlossanlop@users.noreply.github.com>
@carlossanlop

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

This one had lots of changes but I think they were all addressed. @mairaw @rpetrusha if this looks good, can we get it merged?

@mairaw mairaw moved this from In progress to In Review in April 2019 May 6, 2019

@carlossanlop

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

@rpetrusha let me know if this can be merged

@rpetrusha
Copy link
Contributor

left a comment

Here is another installement, @carlossanlop. I still have to provide comments for DiagnosticSource.

Show resolved Hide resolved xml/System.Diagnostics/Activity.xml Outdated
Show resolved Hide resolved xml/System.Diagnostics/Activity.xml Outdated
Show resolved Hide resolved xml/System.Diagnostics/Activity.xml Outdated
Show resolved Hide resolved xml/System.Diagnostics/Activity.xml Outdated
Show resolved Hide resolved xml/System.Diagnostics/Activity.xml Outdated
Show resolved Hide resolved xml/System.Diagnostics/DiagnosticListener.xml Outdated
Show resolved Hide resolved xml/System.Diagnostics/DiagnosticListener.xml Outdated
Show resolved Hide resolved xml/System.Diagnostics/DiagnosticListener.xml Outdated
Show resolved Hide resolved xml/System.Diagnostics/DiagnosticSource.xml
Show resolved Hide resolved xml/System.Diagnostics/Activity.xml Outdated
Apply suggestions from code review
rpetrusha suggestions

Co-Authored-By: Ron Petrusha <ronpet@microsoft.com>
@mairaw

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

There are still some very long descriptions for some parameters and summaries.

@carlossanlop

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

@mairaw @rpetrusha done! Sorry for the delay. The last suggestions were larger and required me to edit the files directly in my machine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.