-
Notifications
You must be signed in to change notification settings - Fork 30k
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
Remove string literal #19276
Remove string literal #19276
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@Nate-weeks would you be so kind and rebase instead of using merge? :-) otherwise it is not possible to run our CI. Even though in this specific case it is probably not necessary to run it. |
@Nate-weeks Please rebase your changes on top of # This is only necessary if you did not add the upstream remote before.
git remote add upstream https://github.com/nodejs/node.git
# Checkout your branch.
git checkout remove-string-literal
# Undo the merge commit.
git reset --hard 10d783c07ca5ac15f013b5bad3709e26cbef87f5
# Make sure your copy of our master branch is up-to-date.
git fetch upstream master
# Rebase on top of our changes.
git rebase upstream/master
|
81443e2
to
2f6d1db
Compare
Oops, yeah sorry that took me awhile to get to and thanks for the instructions, is this better? |
Yes, thank you! CI: https://ci.nodejs.org/job/node-test-pull-request/13691/ |
PR-URL: #19276 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Landed in 92de0eb, thank you @Nate-weeks! 🎉 |
PR-URL: #19276 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Edited several calls to assert.strictEqual in test-crypto-sign-verify.js. I removed third argument string literals that hid the value of
verified
. The default message should include the values.Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes