Skip to content

Conversation

@kevingranade
Copy link

Naming an anonymous function as outlined in #8913

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

@nodejs-github-bot nodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Jul 11, 2018
@trivikr
Copy link
Member

trivikr commented Jul 11, 2018

Thank you @kevingranade for your first PR to Node.js core! 🎉

CI: https://ci.nodejs.org/job/node-test-pull-request/15802/

@tniessen
Copy link
Member

Note that this will conflict with #21153.

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

tniessen commented Jul 13, 2018

This cannot be merged since #21153 has landed, see #21758 (comment). Thank you for your PR anyway, @kevingranade, we are looking forward to more contributions!

@tniessen tniessen closed this Jul 13, 2018
@Trott
Copy link
Member

Trott commented Jul 13, 2018

@kevingranade If you want to try again, staying in the crypto area, here are some small improvements that can be made to test/parallel/test-crypto-binary-default.js:

  • On line 540, a function is declared and takes two arguments. The second argument is never used. Remove it.

  • There are six lines where callbacks are declared using the function keyword. For callbacks, we tend to prefer arrow functions. Change them to be arrow functions.

  • The declaration of DH_NOT_SUITABLE_GENERATOR can be made more concise by using object destructuring.

/cc @trivikr

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. crypto Issues and PRs related to the crypto subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants