Skip to content

Document System.Resources.Extensions #2857

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Aug 1, 2019
Merged

Document System.Resources.Extensions #2857

merged 6 commits into from
Aug 1, 2019

Conversation

carlossanlop
Copy link
Contributor

Here's the PR that introduced these APIs: dotnet/corefx#36906

I automatically ported most of the comments (they existed as triple slash). A few others I copied them (and rewrote them a bit) since they were double slash and could not be ported.

@ericstj @tarekgh @GrabYourPitchforks @rainersigwald can you please verify the descriptions are correct? Particularly the class summaries, I wrote them myself based on what I understood from the code and the PR description. There was no triple slash comment above these new classes. I only found comments above the old ResourceWriter/ResourceReader, but I wasn't really sure if the same description applied for these new extension classes.

@carlossanlop carlossanlop added new-content Indicates PRs that contain new articles waiting-on-reviews Indicates PRs that cannot be merged because of the lack of reviews 🏁 Release: .NET Core 3.0 :checkered_flag: Release: .NET Core 3.0 labels Jul 29, 2019
@carlossanlop carlossanlop added this to the July 2019 milestone Jul 29, 2019
@carlossanlop carlossanlop self-assigned this Jul 29, 2019
rpetrusha
rpetrusha suggested changes Jul 30, 2019
View reviewed changes
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.

I've left a number of suggestions, @carlossanlop.

@carlossanlop
suggestions by rpetrusha
f1b777a 
Co-Authored-By: Ron Petrusha <ronpet@microsoft.com>
@carlossanlop carlossanlop removed the waiting-on-reviews Indicates PRs that cannot be merged because of the lack of reviews label Jul 30, 2019
@carlossanlop
Copy link
Contributor Author

@rpetrusha @mairaw the build passed, all comments have been addressed. Can we get this one merged if you don't have any more comments?

mairaw
mairaw reviewed Jul 30, 2019
View reviewed changes
Copy link
Contributor

@mairaw mairaw left a comment

Choose a reason for hiding this comment

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

A few comments for you to consider... I'll let @rpetrusha approve since he reviewed earlier.

xml/System.Resources.Extensions/DeserializingResourceReader.xml
<summary>To be added.</summary>
<returns>To be added.</returns>
<summary>Returns an enumerator for this <see cref="T:System.Resources.Extensions.DeserializingResourceReader" /> object.</summary>
<returns>An enumerator for this <see cref="T:System.Resources.Extensions.DeserializingResourceReader" /> object.</returns>
<remarks>To be added.</remarks>
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add the remarks here as we did in other PRs about EII?
I'll add that to the wiki as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you please provide a remark suggestion for EII?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we do this in a separate PR, @mairaw?

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem. I've added the boilerplate text to our writing guidelines here: https://github.com/dotnet/dotnet-api-docs/wiki/Remarks#explicit-interface-implementation

carlossanlop
carlossanlop commented Jul 31, 2019
View reviewed changes
@carlossanlop @mairaw
suggestions by mairaw
95e56a8 
Co-Authored-By: Maira Wenzel <mairaw@microsoft.com>
@carlossanlop
Copy link
Contributor Author

One more request for @ericstj @tarekgh @GrabYourPitchforks @rainersigwald : The namespace System.Resources.Extensions is also missing a summary. Would you mind writing one so I can submit it in a separate PR?

ericstj
ericstj reviewed Jul 31, 2019
View reviewed changes
carlossanlop
carlossanlop commented Jul 31, 2019
View reviewed changes
carlossanlop
carlossanlop commented Jul 31, 2019
View reviewed changes
carlossanlop
carlossanlop commented Jul 31, 2019
View reviewed changes
carlossanlop
carlossanlop commented Jul 31, 2019
View reviewed changes
carlossanlop
carlossanlop commented Jul 31, 2019
View reviewed changes
carlossanlop
carlossanlop commented Jul 31, 2019
View reviewed changes
carlossanlop
carlossanlop commented Jul 31, 2019
View reviewed changes
carlossanlop
carlossanlop commented Jul 31, 2019
View reviewed changes
carlossanlop
carlossanlop commented Jul 31, 2019
View reviewed changes
carlossanlop
carlossanlop commented Jul 31, 2019
View reviewed changes
carlossanlop
carlossanlop commented Jul 31, 2019
View reviewed changes
carlossanlop
carlossanlop commented Jul 31, 2019
View reviewed changes
carlossanlop
carlossanlop commented Jul 31, 2019
View reviewed changes
carlossanlop
carlossanlop commented Jul 31, 2019
View reviewed changes
carlossanlop
carlossanlop commented Jul 31, 2019
View reviewed changes
carlossanlop
carlossanlop commented Jul 31, 2019
View reviewed changes
@carlossanlop carlossanlop requested a review from ericstj July 31, 2019 23:55
ericstj
ericstj approved these changes Jul 31, 2019
View reviewed changes
Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

OK with minor suggestions

@carlossanlop @ericstj
suggestions by ericstj
2e504ab 
Co-Authored-By: Eric StJohn <ericstj@microsoft.com>
@carlossanlop carlossanlop added the changes-addressed Indicates PRs that had all comments addressed and are awaiting for new review label Aug 1, 2019
@carlossanlop carlossanlop requested a review from rpetrusha August 1, 2019 04:13
@rpetrusha
Copy link

I'll merge this PR now, @carlossanlop.

@rpetrusha rpetrusha merged commit 9fdf549 into dotnet:master Aug 1, 2019
@carlossanlop carlossanlop deleted the SystemResourcesExtensions branch August 1, 2019 16:53
@mairaw mairaw removed the changes-addressed Indicates PRs that had all comments addressed and are awaiting for new review label Aug 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tarekgh tarekgh tarekgh approved these changes

@mairaw mairaw mairaw left review comments

@joperezr joperezr joperezr left review comments

@ericstj ericstj ericstj approved these changes

@GrabYourPitchforks GrabYourPitchforks Awaiting requested review from GrabYourPitchforks

@rainersigwald rainersigwald Awaiting requested review from rainersigwald

+1 more reviewer

@rpetrusha rpetrusha rpetrusha approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@carlossanlop carlossanlop

Labels
🏁 Release: .NET Core 3.0 :checkered_flag: Release: .NET Core 3.0 new-content Indicates PRs that contain new articles
Projects
None yet
Milestone
July 2019
Development

Successfully merging this pull request may close these issues.
6 participants
@carlossanlop @rpetrusha @ericstj @tarekgh @mairaw @joperezr