-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Automatically port System.IO triple slash comments #2675
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
Conversation
|
@JeremyKuhne can you please double check the comments I added are correct? |
mairaw
left a comment
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.
My review comments so far. Need to continue the review starting at FileSystemName.xml.
JeremyKuhne
left a comment
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.
Some initial feedback on technical correctness.
Co-Authored-By: Maira Wenzel <[email protected]>
|
I'll start re-reviewing this one now but the build report has a couple of broken links for this PR. Do you want to take a look @carlossanlop? |
| <param name="expression">The expression to match with.</param> | ||
| <param name="name">The name to check against the expression.</param> | ||
| <param name="ignoreCase"><see langword="true" /> to ignore case (default), <see langword="false" /> if the match should be case-sensitive.</param> | ||
| <summary>Verifies if the given expression matches the given name. Supports the following wildcards: '*' and '?'. The backslash character '\' escapes.</summary> |
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 \ character is not rendering on docs. I'll need to check with the eng team about this. Probably escaping would fix it for docs but could break IntelliSense.
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.
Could we use its html entity?
https://www.freeformatter.com/html-entities.html
\
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.
We can try. Should we merge this and figure out on a separate PR how to fix 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.
Okay sure, let's get it merged. I can address it separately.
|
@carlossanlop @JeremyKuhne do you want to review the changes I made? |
mairaw
left a comment
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.
LGTM after some edits
|
I reviewed your commits, thank you for making those changes. They look good. |
|
I've fixed the broken crefs, so this should be ready to merge when the build completes. |
Found a new batch of System.IO triple slash comments that can be ported. Also manually added a few others.