Skip to content

Add missing, and update existing, documentation on the RSA classes #3660

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.

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

Merged
merged 4 commits into from
Dec 12, 2019

Conversation

bartonjs
Copy link
Member

@dotnet-bot dotnet-bot added this to the December 2019 milestone Dec 11, 2019
@carlossanlop carlossanlop requested review from jozkee, carlossanlop and a team December 11, 2019 16:58
@carlossanlop carlossanlop added 🏁 Release: .NET Core 2.x Identifies work items for the .NET Core 2.x releases assigned-to-author Assigned to the author to resolve 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 11, 2019
<returns>To be added.</returns>
<remarks>To be added.</remarks>
<param name="hashAlgorithm">The hash algorithm used to create the hash value of the data.</param>
<param name="padding">The padding.</param>
Copy link
Contributor

Choose a reason for hiding this comment

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

This parameter could use a better description.

<returns>To be added.</returns>
<param name="hash">The hash value of the data to be signed.</param>
<param name="hashAlgorithm">The hash algorithm used to create the hash value of the data.</param>
<param name="padding">The padding.</param>
Copy link
Contributor

Choose a reason for hiding this comment

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

This parameter could use a better description.

<param name="hash">The hash value of the signed data.</param>
<param name="signature">The signature data to be verified.</param>
<param name="hashAlgorithm">The hash algorithm used to create the hash value.</param>
<param name="padding">The padding mode.</param>
Copy link
Contributor

Choose a reason for hiding this comment

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

This parameter has one extra word, but I think it could also be described better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same suggestion applies to the rest of the padding params.

Copy link
Member Author

Choose a reason for hiding this comment

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

The "padding" parameter descriptions are the ones that are already present on the documented overloads and variants (e.g. https://docs.microsoft.com/en-us/dotnet/api/system.security.cryptography.rsa.signdata?view=netframework-4.8).

They can be long winded, such as "The padding mode to use for the {encryption|decryption|signing|verification} process.", but there's really nothing else that I can think of to say about them. I'm open to suggestions, but absent a specific suggestion I'm going to leave them as-is.

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.

It looks good. I left a suggestion to improve the padding param description.

@bartonjs
Copy link
Member Author

Looks like there were some bad cref/xrefs, I'll get them after lunch (assuming no one does it before then).

@BillWagner
Copy link
Member

@bartonjs This now has merge conflicts. Can you resolve those?

# Conflicts:
#	xml/System.Security.Cryptography/RSACng.xml
@bartonjs
Copy link
Member Author

@BillWagner Conflicts resolved, bad crefs/xrefs resolved.

@carlossanlop carlossanlop merged commit 87b39dd into dotnet:master Dec 12, 2019
@bartonjs bartonjs deleted the rsa branch December 12, 2019 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned-to-author Assigned to the author to resolve 🏁 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants