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

crypto: naming anonymouse functions #8993

Closed
wants to merge 1 commit into from

Conversation

solebox
Copy link
Contributor

@solebox solebox commented Oct 9, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

crypto

Description of change

rename anonymouse functions
Ref: #8913

@nodejs-github-bot nodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Oct 9, 2016
@solebox
Copy link
Contributor Author

solebox commented Oct 9, 2016

fixes some of it , not the whole thing. as requested in the thread, pull request per module.

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Generally, this looks fine! Some of the lines need to be wrapped, and there’s a anonymouse typo in the commit message.

return binding.certVerifySpkac(object);
};


Certificate.prototype.exportPublicKey = function(object, encoding) {
Certificate.prototype.exportPublicKey = function exportPublicKey(object, encoding) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you check that your lines are no longer than 80 characters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@addaleax
Copy link
Member

addaleax commented Oct 9, 2016

Oh, and if your commit only partially solves a Github issue, it’s better to use use Ref: than to use Fixes:, so that landing the PR doesn’t automatically close the issue.

@solebox
Copy link
Contributor Author

solebox commented Oct 9, 2016

submitted changes

@AndreasMadsen
Copy link
Member

AndreasMadsen commented Oct 9, 2016

Are methods on the prototype object actually a problem? In a normal stack trace they will have a full name e.g. Hash.update, even if the function is technically anonymous.

function C() {}
C.prototype.method = function () { throw new Error(''); }
new C().method()
Error
    at C.method (repl:1:42)
    ...

@solebox
Copy link
Contributor Author

solebox commented Oct 9, 2016

@AndreasMadsen when requesting .name i got an empty string.
that was the means of testing offered in the original issue posted.

i assume that mattered to some people when debugging

(check out the repl feedback , its pretty nice to have name set when you're using repl, really helps)

return binding.certVerifySpkac(object);
};


Certificate.prototype.exportPublicKey = function(object, encoding) {
Certificate.prototype.exportPublicKey = function exportPublicKey(object,
encoding) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Typically we like to keep the parameters all lined up if on multiple lines. However in the case that a lined up parameter on a new line would exceed the 80 character mark, it would probably be better to just move everything after the = to the next line and indent the entire function definition by 2 spaces.

Ditto for the changes below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok ill do that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM if CI is green.

@targos
Copy link
Member

targos commented Oct 10, 2016

LGTM

Copy link
Contributor

@thefourtheye thefourtheye left a comment

Choose a reason for hiding this comment

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

Change looks okay, but commit message could use the following changes

  1. typo should be fixed and
  2. if possible, an imperative verb could be used as the first word. For example, crypto: name anonymous functions

@jasnell
Copy link
Member

jasnell commented Oct 11, 2016

@solebox
Copy link
Contributor Author

solebox commented Oct 12, 2016

@jasnell hello james , my commit only changes javascript i dont see a reason why it wont work on all platforms just the same.

looks like test-npm-install.js is the culprit (at least in the free-bsd box, arm has no report yet)
doesnt look like it has something to do with my changes
am i missing something here?

@gibfahn
Copy link
Member

gibfahn commented Oct 12, 2016

@soleboxy Those failures look like infrastructure issues.

@gibfahn
Copy link
Member

gibfahn commented Oct 12, 2016

New CI (we'll run it until it works ) https://ci.nodejs.org/job/node-test-pull-request/4490/

Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM

@addaleax
Copy link
Member

@addaleax addaleax self-assigned this Oct 15, 2016
@addaleax
Copy link
Member

I’ll start landing this:

  • Approvals (LGTM): 5
  • No objections
  • The PR has been open for the minimum time of 48 or 72 hours
  • All of the requested changes have been made
  • CI has only unrelated failures

@addaleax
Copy link
Member

You author name in this commit is given as “solebox”. Is that intended or do you prefer to be listed (changelog, git log, AUTHORS file) with some other name? People typically prefer their full name, but ultimately it’s up to you.

@addaleax
Copy link
Member

Landed in 6f05de4! 🎉

@addaleax addaleax closed this Oct 15, 2016
addaleax pushed a commit that referenced this pull request Oct 15, 2016
Ref: #8913
PR-URL: #8993
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell pushed a commit that referenced this pull request Oct 17, 2016
Ref: #8913
PR-URL: #8993
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins
Copy link
Contributor

I'm going to hold off on backporting this at the moment, but I'm very curious where we landed on this and thoughts on backporting to v6

@MylesBorins MylesBorins added the baking-for-lts PRs that need to wait before landing in a LTS release. label May 9, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@sam-github
Copy link
Contributor

I think we should land it, it looks very safe, its baked for a while, and not having landed caused #11705 to not cherry-pick clean, triggering a backport, #14376.

@solebox
Copy link
Contributor Author

solebox commented Jul 27, 2017

thanks :)

@sam-github
Copy link
Contributor

@MylesBorins I pushed this onto v6.x-staging, it cherry picks clean, and will help @tniessen with #14376 which we asked for.

Landed in e9235e8e2efb0443d0d770a7bdce4aff983fdbc9 for v6.x-staging.

MylesBorins pushed a commit that referenced this pull request Sep 20, 2017
@MylesBorins MylesBorins mentioned this pull request Sep 20, 2017
@MylesBorins MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.