Skip to content

Automatically port Reflection.Metadata.Ecma335 Method* APIs #3211

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 4 commits into from
Oct 2, 2019
Merged

Automatically port Reflection.Metadata.Ecma335 Method* APIs #3211

merged 4 commits into from
Oct 2, 2019

Conversation

carlossanlop
Copy link
Contributor

Area owners: @tmat, @nguerrera

@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 2.x Identifies work items for the .NET Core 2.x releases labels Sep 18, 2019
@carlossanlop carlossanlop added this to the September 2019 milestone Sep 18, 2019
@carlossanlop carlossanlop self-assigned this Sep 18, 2019
@carlossanlop carlossanlop requested review from tmat and removed request for arpitmathur September 18, 2019 22:57
rpetrusha
rpetrusha suggested changes Sep 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 some comments and suggestions, @carlossanlop.

xml/System.Reflection.Metadata.Ecma335/MethodSignatureEncoder.xml
@@ -124,10 +124,10 @@
<Parameter Name="parameters" Type="System.Action&lt;System.Reflection.Metadata.Ecma335.ParametersEncoder&gt;" Index="2" FrameworkAlternate="netcore-1.1;netcore-2.0;netcore-2.1;netcore-2.2;netcore-3.0" />
Copy link

Choose a reason for hiding this comment

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

There should be a type description (like "Provides an encoder for method signatures.")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There should be a type description (like "Provides an encoder for method signatures.")

I will add your suggestion, @rpetrusha.
@tmat @nguerrera if you prefer a different summary for the MethodSignatureEncoder type, please let me know.

carlossanlop and others added 3 commits September 30, 2019 15:34
@carlossanlop
suggestions by rpetrusha
d84ea06 
Co-Authored-By: Ron Petrusha <ronpet@microsoft.com>
@carlossanlop
Disambiguate constructors
527bcce
@carlossanlop
Type descriptions
d81027e
@carlossanlop carlossanlop added changes-addressed Indicates PRs that had all comments addressed and are awaiting for new review and removed waiting-on-reviews Indicates PRs that cannot be merged because of the lack of reviews labels Sep 30, 2019
@rpetrusha
Copy link

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

@rpetrusha rpetrusha merged commit 46ed09f into dotnet:master Oct 2, 2019
@carlossanlop carlossanlop deleted the Ecma335_Method branch October 2, 2019 20:58
@mairaw mairaw removed the changes-addressed Indicates PRs that had all comments addressed and are awaiting for new review label Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nguerrera nguerrera Awaiting requested review from nguerrera

@mairaw mairaw Awaiting requested review from mairaw

@tmat tmat Awaiting requested review from tmat

1 more reviewer

@rpetrusha rpetrusha rpetrusha approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@carlossanlop carlossanlop

Labels
🏁 Release: .NET Core 2.x Identifies work items for the .NET Core 2.x releases new-content Indicates PRs that contain new articles
Projects
None yet
Milestone
September 2019
Development

Successfully merging this pull request may close these issues.
3 participants
@carlossanlop @rpetrusha @mairaw