Update docs for new Syndication APIs in 2.x #3743
Conversation
There was a problem hiding this comment.
Overall, this looks great @StephenMolloy
I reviewed everything, made a few suggestions, and had a couple style guide questions for @mairaw
I'll remove myself as a reviewer, and add her so she adds clarification on those questions.
Then, this should be ready to merge.
| <remarks>To be added.</remarks> | ||
| <param name="dateTimeString">The raw string representation of the timestamp.</param> | ||
| <param name="elementQualifiedName">The qualified name of the xml element that originally contained the raw timestamp string.</param> | ||
| <summary>Sets the DateTimeString and ElementQualifiedName properties using the given parameters.</summary> |
There was a problem hiding this comment.
I'd either cref, or format the DateTimeString and ElementQualifiedName as code.
There was a problem hiding this comment.
@BillWagner I would be more inclined to use natural language instead. Those two elements are already parameters, and the parameters already have their data type right next to them. Formatting them as code (I think you mean <c></c>) seems unusual.
@mairaw what do you think? For this and all the rest of the suggestions below.
There was a problem hiding this comment.
I resolved all the other feedback so far, but the question about cref vs <c> for these elements of XmlDateTimeData.xml and XmlUriData.xml is still an open question. They are currently all cref. Should they switch to before approving the PR?
There was a problem hiding this comment.
I talked to Maira and we should use cref.
There was a problem hiding this comment.
I noticed you already did, great!
| <summary>To be added.</summary> | ||
| <value>To be added.</value> | ||
| <remarks>To be added.</remarks> | ||
| <summary>Gets the DateTimeString property.</summary> |
There was a problem hiding this comment.
same comment: DateTimeString should be formatted as code.
| <summary>To be added.</summary> | ||
| <value>To be added.</value> | ||
| <remarks>To be added.</remarks> | ||
| <summary>Gets the ElementQualifiedName property.</summary> |
There was a problem hiding this comment.
The ElementQualifiedName should be formatted as code.
| <summary>To be added.</summary> | ||
| <value>To be added.</value> | ||
| <remarks>To be added.</remarks> | ||
| <summary>Gets the UriString property.</summary> |
There was a problem hiding this comment.
The UriString should be formatted as code.
carlossanlop
added
🏁 Release: .NET Core 2.x
There was a problem hiding this comment.
Thank you for these changes, @StephenMolloy ! I found some build warnings in the CI which need to be addressed to unblock merging. I provided suggestions to get them fixed.
Resolve CI warnings. Co-Authored-By: Carlos Sanchez Lopez <1175054+carlossanlop@users.noreply.github.com>
Some text and formatting cleanup from review. Co-Authored-By: Bill Wagner <wiwagn@microsoft.com>
Co-Authored-By: Maira Wenzel <mairaw@microsoft.com>
Done. :) |
Co-Authored-By: Carlos Sanchez Lopez <1175054+carlossanlop@users.noreply.github.com>
mairaw
changed the title
mairaw
changed the title
Summary
Add documentation for new Syndication API's added in 2.X.