Repository navigation
Match the documented parameter names to the signatures - #463
Conversation
simonjbeaumont
left a comment
simonjbeaumont
left a comment
There was a problem hiding this comment.
@darkdi thanks for taking the time to open this PR 🙏
In general these housekeeping PRs are usually very welcome -- it's good for us to have the doc comments line up properly, especially for public API -- however, because this project shares code with another changes the tradeoffs.
Files that do not have the _boring or _xkcp suffix are part of the public API of CryptoKit. Changing these requires passing a higher bar, as any change in these files must be accompanied by a change in CryptoKit itself.
Could you revert the changes made to the files in Crypto/ so that the patch only affects the changes in CryptoExtras/?
There's no user benefit to those changes anyway, since those documentation comments are not on public symbols. Conversely, one of the changes you made in CryptoExtras is useful since it fixes the broken documentation on public API 👍
simonjbeaumont
added
area/documentation
15b1190 to
f1f67ae
Compare
|
Reverted, the patch touches only CryptoExtras now. I missed the code organisation note in the README about the files without the |
simonjbeaumont
left a comment
simonjbeaumont
left a comment
There was a problem hiding this comment.
Reverted, the patch touches only CryptoExtras now. I missed the code organisation note in the README about the files without the
_boringsuffix, that is on me.
No worries at all. And thank you for the contribution 🙏
Head branch was pushed to by a user without write access
|
The two workflow runs for this head are sitting in |
|
No worries. I'll take care of this tomorrow. |
Match the documented parameter names to the signatures in Crypto and CryptoExtras
Checklist
Motivation:
Both
AES.GCM._SIV.openoverloads document a nonce, with the paragraph about keeping it unique for every use of the key, and neither of them takes one -- it arrives inside the sealed box. That paragraph is copied down fromseal, where it belongs. In a crypto API this seems worse than a typo, since it sends the reader looking for a parameter that is not there.Cipher.opencarries the same copy. The other four look like plain name drift.Modifications:
Dropped the three nonce entries, renamed four entries to the names the signatures use. Comments only, no code touched.
Result:
The parameter lists match the signatures.