crypto: allow undefined for saltLength and padding by tniessen · Pull Request #26921 · nodejs/node (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation7 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Using options.hasOwnProperty is not how we usually validate options. Passing { saltLength } where saltLength === undefined should be equivalent to not passing a saltLength option in my opinion. Feel free to disagree!
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes- commit message follows commit guidelines
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree.
PRs that have at least one approval, no pending requests for changes, and a CI started.
label
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. For reference, the hasOwnProperty() check was introduced in #11705.
I agree. The only reason i can come up with is that providing undefined as the property's value is indicative of users thinking they pass the constant value while in fact they're not, so before this PR their error would be caught right there, now it won't and the defaults will be applied.
This comment has been minimized.
Thanks for reviewing, landed in c35acc0.
tniessen added a commit that referenced this pull request
PR-URL: #26921 Reviewed-By: Sam Roberts vieuxtech@gmail.com Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Ben Noordhuis info@bnoordhuis.nl
migounette pushed a commit to migounette/node-js that referenced this pull request
PR-URL: nodejs#26921 Reviewed-By: Sam Roberts vieuxtech@gmail.com Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Ben Noordhuis info@bnoordhuis.nl
targos pushed a commit that referenced this pull request
PR-URL: #26921 Reviewed-By: Sam Roberts vieuxtech@gmail.com Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Ben Noordhuis info@bnoordhuis.nl
BethGriggs pushed a commit that referenced this pull request
PR-URL: #26921 Reviewed-By: Sam Roberts vieuxtech@gmail.com Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Ben Noordhuis info@bnoordhuis.nl
BethGriggs pushed a commit that referenced this pull request
PR-URL: #26921 Reviewed-By: Sam Roberts vieuxtech@gmail.com Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Ben Noordhuis info@bnoordhuis.nl Signed-off-by: Beth Griggs Bethany.Griggs@uk.ibm.com
PRs that have at least one approval, no pending requests for changes, and a CI started.
label
Labels
Issues and PRs related to the crypto subsystem.