Skip to content

Add/update documentation for the X509Certificates namespace. #3669

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 5 commits into from
Dec 18, 2019
Merged

Add/update documentation for the X509Certificates namespace. #3669

merged 5 commits into from
Dec 18, 2019

Conversation

bartonjs
Copy link
Member

With this change, there are zero <summary>To be added.</summary> remaining in the namespace directory.

cc: @carlossanlop

@dotnet-bot dotnet-bot added this to the December 2019 milestone Dec 12, 2019
@carlossanlop carlossanlop requested review from jozkee, carlossanlop and a team December 12, 2019 21:10
@carlossanlop carlossanlop added 🏁 Release: .NET Core 2.x Identifies work items for the .NET Core 2.x releases new-content Indicates PRs that contain new articles waiting-on-reviews Indicates PRs that cannot be merged because of the lack of reviews labels Dec 12, 2019
carlossanlop
carlossanlop reviewed Dec 12, 2019
View reviewed changes
xml/System.Security.Cryptography.X509Certificates/X509Certificate.xml
@@ -2656,6 +2671,7 @@
<permission cref="T:System.Security.SecurityCriticalAttribute">requires full trust for the immediate caller. This class cannot be used by partially trusted or transparent code.</permission>
<permission cref="F:System.Security.Permissions.SecurityAction.InheritanceDemand">for full trust for inheritors. This member cannot be inherited by partially trusted code.</permission>
<permission cref="T:System.Security.Permissions.KeyContainerPermission">for permission to create a key container. Security action: <see cref="F:System.Security.Permissions.SecurityAction.Demand" />. Associated enumeration: <see cref="F:System.Security.Permissions.KeyContainerPermissionFlags.Create" /></permission>
<exception cref="T:System.PlatformNotSupportedException">.NET Core only: In all cases.</exception>
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 a remark to all these APIs indicating the reason why we throw PlatformNotSupportedException?

carlossanlop
carlossanlop reviewed Dec 12, 2019
View reviewed changes
xml/System.Security.Cryptography.X509Certificates/X509CertificateCollection.xml
<summary>To be added.</summary>
<param name="index">The zero-based index at which the value should be inserted.</param>
<param name="value">The object to insert.</param>
<summary>Inserts an element into the collection at the specified index.</summary>
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this method throw if the index is beyond the limits of the list?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably, but I can't tell you what it threw. (This explicit implementation was deleted in netcoreapp2.0/netstandard2.0, when CollectionBase was brought back... which is true for the all of the cases where you had exception/behavior questions)

carlossanlop
carlossanlop reviewed Dec 12, 2019
View reviewed changes
xml/System.Security.Cryptography.X509Certificates/X509CertificateCollection.xml
<value>To be added.</value>
<param name="index">The zero-based index of the element to get or set.</param>
<summary>Gets or sets the element at the specified index.</summary>
<value>The element at the specified index.</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this method throw if the index is beyond the limits of the list?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll assume the answer to my question is the same as above. Feel free to ignore.

carlossanlop
carlossanlop reviewed Dec 12, 2019
View reviewed changes
xml/System.Security.Cryptography.X509Certificates/X509CertificateCollection.xml
<param name="value">To be added.</param>
<summary>To be added.</summary>
<param name="value">The object to remove from the collection.</param>
<summary>Removes the first occurrence of a specific object from the collection.</summary>
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this method behave if there's no object with this value?

carlossanlop
carlossanlop suggested changes Dec 12, 2019
View reviewed changes
Copy link
Contributor

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

Thanks for making this change (last for 2.X!). Left a few suggestions for you to consider.

@carlossanlop carlossanlop removed the waiting-on-reviews Indicates PRs that cannot be merged because of the lack of reviews label Dec 12, 2019
bartonjs and others added 2 commits December 12, 2019 13:44
@bartonjs @carlossanlop
Apply suggestions from code review
2907490 
Co-Authored-By: Carlos Sanchez Lopez <1175054+carlossanlop@users.noreply.github.com>
@bartonjs @carlossanlop
Apply suggestions from code review
89b3cae 
Co-Authored-By: Carlos Sanchez Lopez <1175054+carlossanlop@users.noreply.github.com>
gewarren
gewarren approved these changes Dec 16, 2019
View reviewed changes
Copy link
Contributor

@gewarren gewarren left a comment

Choose a reason for hiding this comment

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

Approved with suggestions.

gewarren
gewarren reviewed Dec 16, 2019
View reviewed changes
Copy link
Contributor

@gewarren gewarren left a comment

Choose a reason for hiding this comment

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

One more suggestion to replace a previous suggestion.

@bartonjs @gewarren
Apply suggestions from code review
eb06cf4 
Co-Authored-By: Genevieve Warren <gewarren@microsoft.com>
carlossanlop
carlossanlop approved these changes Dec 18, 2019
View reviewed changes
Copy link
Contributor

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

@bartonjs I think the PR looks good enough as it is. If you feel like addressing the open questions, let's do it in a separate PR. I'll merge this.

@carlossanlop carlossanlop merged commit 36f312b into dotnet:master Dec 18, 2019
@bartonjs bartonjs deleted the x509 branch December 19, 2019 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gewarren gewarren gewarren approved these changes

@jozkee jozkee Awaiting requested review from jozkee

+1 more reviewer

@carlossanlop carlossanlop carlossanlop approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@bartonjs bartonjs

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
December 2019
Development

Successfully merging this pull request may close these issues.
4 participants
@bartonjs @carlossanlop @gewarren @dotnet-bot