Skip to content

coreclr: Automatic port of triple slash from System.IO #2724

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 10 commits into from
Aug 5, 2019
Merged

coreclr: Automatic port of triple slash from System.IO #2724

merged 10 commits into from
Aug 5, 2019

Conversation

carlossanlop
Copy link
Contributor

Area owners @JeremyKuhne and myself. Please make sure the descriptions are correct. After I check-in this PR, please feel free to start manually documenting the rest of the APIs that didn't have any triple slash comments that could be automatically ported.
Adding @mairaw @rpetrusha for language review.

mairaw
mairaw requested changes Jul 8, 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.

Looks good @carlossanlop. Left some comments to be addressed. In some cases, I've suggested changes to make the APIs consistent with the docs for similar overloads already published.

@mairaw mairaw added new-content Indicates PRs that contain new articles 🏁 Release: .NET Core 3.0 :checkered_flag: Release: .NET Core 3.0 labels Jul 8, 2019
@mairaw mairaw added this to the July 2019 milestone Jul 8, 2019
@carlossanlop @mairaw
Suggestions by mairaw
ac68a20 
Co-Authored-By: Maira Wenzel <mairaw@microsoft.com>
@carlossanlop carlossanlop added the changes-addressed Indicates PRs that had all comments addressed and are awaiting for new review label Jul 9, 2019
@carlossanlop carlossanlop requested a review from mairaw July 9, 2019 23:49
carlossanlop
carlossanlop commented Jul 12, 2019
View reviewed changes
carlossanlop
carlossanlop commented Jul 12, 2019
View reviewed changes
@carlossanlop carlossanlop requested a review from rpetrusha July 12, 2019 22:10
@carlossanlop
Copy link
Contributor Author

@rpetrusha added a couple missing returns. While Maira is gone, if you have a chance to take a look at this one, that would be great.

carlossanlop
carlossanlop commented Jul 12, 2019
View reviewed changes
@carlossanlop
Copy link
Contributor Author

I resolved the merge conflicts with the latest on master.
Turns out I had already submitted (and merged) the updates for TextWriter.xml, so I took the merged changes, and removed the file from this PR.

@mairaw
Copy link
Contributor

mairaw commented Jul 24, 2019

I think something went wrong with the merge conflict resolution. In some cases, the API lost its description.

There were two recent changes to the UnmanagedMemoryStream.xml file: PR #2744 and #2776. Is there a particular version that you like better?

@carlossanlop
Copy link
Contributor Author

@mairaw okay I made a mess before, but I think I fixed it. I was not using the Visual Studio merge tool correctly. I made sure to apply your suggestions and the commits you added yourself. Can you please take another look?

@carlossanlop
Copy link
Contributor Author

The build passed. One final check before we merge, @mairaw @rpetrusha ?

mairaw
mairaw approved these changes Aug 2, 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.

LGTM. Made some minor changes that you might want to review.

@mairaw mairaw removed the changes-addressed Indicates PRs that had all comments addressed and are awaiting for new review label Aug 2, 2019
@rpetrusha
Copy link

Thanks for the additional changes, @mairaw. They look good.

@rpetrusha rpetrusha merged commit 684dbf4 into dotnet:master Aug 5, 2019
@carlossanlop carlossanlop deleted the clr_System.IO branch September 22, 2020 00:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mairaw mairaw mairaw approved these changes

@rpetrusha rpetrusha Awaiting requested review from rpetrusha

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.
3 participants
@carlossanlop @mairaw @rpetrusha