Skip to content

Conversation

carlossanlop
Copy link
Contributor

Summary

Automatically porting a few triple slash source code comments found in System.Linq.Expressions that were not yet documented in dotnet-api-docs.

Fixes #Issue_Number (if available)

@carlossanlop
Copy link
Contributor Author

@mairaw @rpetrusha please take a look whenever possible.

Adding the area owners @cston, @333fred. Charles, Fred, can you please take a look? The summary messages I ported are all generic and have exactly the same string, so it would be awesome if the code experts could jump in to edit them into some more descriptive and specific summaries.

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming that I'm reading the XML correctly and this adds docs to a bunch of Accept methods, these look fine to me.

@mairaw mairaw added this to the April 2019 milestone Apr 26, 2019
@carlossanlop
Copy link
Contributor Author

@mairaw @rpetrusha if this looks good enough, can we get it merged?

Copy link

@rpetrusha rpetrusha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The summaries look good, @carlossanlop, but the parameter descriptions and return values are missing. Would it be possible for you to also add those? The parameter description is, "The visitor to visit this node with." The return value description is "The result of visiting this node."

@carlossanlop
Copy link
Contributor Author

The summaries look good, @carlossanlop, but the parameter descriptions and return values are missing. Would it be possible for you to also add those? The parameter description is, "The visitor to visit this node with." The return value description is "The result of visiting this node."

For the visitor parameter, would it sound better to use the following instead?:

"The <see cref="T:System.Linq.Expressions.ExpressionVisitor"> with which this node will be visited."

Suggested by rpetrusha
@carlossanlop
Copy link
Contributor Author

@rpetrusha done.

@rpetrusha
Copy link

Thanks for making all of the additional changes, @carlossanlop. I'll merge your PR now.

@rpetrusha rpetrusha merged commit 7e2b7fe into dotnet:master May 3, 2019
@carlossanlop
Copy link
Contributor Author

Thank you @rpetrusha !

@carlossanlop carlossanlop deleted the linqexpressions branch May 3, 2019 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants