Skip to content
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

lib: use object destructuring for canonicalizeIP #20070

Closed

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Apr 16, 2018

This commit updates canonicalizeIP to use object destructuring which is
done for other functions in lib/tls.js

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

This commit updates canonicalizeIP to use object destructuring which is
done for other functions in lib/tls.js
@nodejs-github-bot nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label Apr 16, 2018
@danbev
Copy link
Contributor Author

danbev commented Apr 16, 2018

@lpinca lpinca added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 16, 2018
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM with the other variable using destructuring as well.

lib/tls.js Outdated
@@ -33,7 +33,7 @@ const binding = process.binding('crypto');
const Buffer = require('buffer').Buffer;
Copy link
Member

Choose a reason for hiding this comment

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

This should also use destructuring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I missed that one. I'll update shortly.

@danbev
Copy link
Contributor Author

danbev commented Apr 18, 2018

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

Still LGTM

@danbev
Copy link
Contributor Author

danbev commented Apr 19, 2018

node-test-commit failure looks unrelated

console output:

06:36:40 not ok 2170 es-module/test-esm-preserve-symlinks-not-found
06:36:40   ---
06:36:40   duration_ms: 0.264
06:36:40   severity: crashed
06:36:40   exitcode: -6
06:36:40   stack: |-
06:36:40     oh no!
06:36:40     exit code: CRASHED (Signal: 6)
06:36:40   ...

@danbev
Copy link
Contributor Author

danbev commented Apr 19, 2018

Landed in 72d1586.

@danbev danbev closed this Apr 19, 2018
@danbev danbev deleted the canonicalizeIP-object-destructuring branch April 19, 2018 03:34
danbev added a commit that referenced this pull request Apr 19, 2018
This commit updates canonicalizeIP and Buffer to use object
destructuring which is done for other functions in lib/tls.js

PR-URL: #20070
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
jasnell pushed a commit that referenced this pull request Apr 19, 2018
This commit updates canonicalizeIP and Buffer to use object
destructuring which is done for other functions in lib/tls.js

PR-URL: #20070
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants