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
base: master
from

Conversation

@solebox
Contributor

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

@solebox

This comment has been minimized.

Contributor

solebox commented Oct 9, 2016

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

@addaleax

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

lib/crypto.js Outdated
return binding.certVerifySpkac(object);
};
Certificate.prototype.exportPublicKey = function(object, encoding) {
Certificate.prototype.exportPublicKey = function exportPublicKey(object, encoding) {

This comment has been minimized.

@addaleax

addaleax Oct 9, 2016

Member

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

This comment has been minimized.

@solebox

solebox Oct 10, 2016

Contributor

done

@addaleax

This comment has been minimized.

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 solebox force-pushed the solebox:rename_functions branch Oct 9, 2016

@solebox

This comment has been minimized.

Contributor

solebox commented Oct 9, 2016

submitted changes

@AndreasMadsen

This comment has been minimized.

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

This comment has been minimized.

Contributor

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)

lib/crypto.js Outdated
return binding.certVerifySpkac(object);
};
Certificate.prototype.exportPublicKey = function(object, encoding) {
Certificate.prototype.exportPublicKey = function exportPublicKey(object,
encoding) {

This comment has been minimized.

@mscdex

mscdex Oct 9, 2016

Contributor

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.

This comment has been minimized.

@solebox

solebox Oct 9, 2016

Contributor

ok ill do that

This comment has been minimized.

@solebox

solebox Oct 9, 2016

Contributor

done

@solebox solebox force-pushed the solebox:rename_functions branch Oct 9, 2016

@jasnell

LGTM if CI is green.

@targos

This comment has been minimized.

Member

targos commented Oct 10, 2016

LGTM

@thefourtheye

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

This comment has been minimized.

@solebox

This comment has been minimized.

Contributor

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

This comment has been minimized.

Member

gibfahn commented Oct 12, 2016

@soleboxy Those failures look like infrastructure issues.

@gibfahn

This comment has been minimized.

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/

@lpinca

lpinca approved these changes Oct 12, 2016

LGTM

@solebox solebox force-pushed the solebox:rename_functions branch to 72be5ed Oct 13, 2016

@addaleax

LGTM

@addaleax

This comment has been minimized.

Member

addaleax commented Oct 15, 2016

@addaleax addaleax self-assigned this Oct 15, 2016

@addaleax

This comment has been minimized.

Member

addaleax commented Oct 15, 2016

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

This comment has been minimized.

Member

addaleax commented Oct 15, 2016

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

This comment has been minimized.

Member

addaleax commented Oct 15, 2016

Landed in 6f05de4! 🎉

@addaleax addaleax closed this Oct 15, 2016

addaleax added a commit that referenced this pull request Oct 15, 2016

crypto: naming anonymous functions
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 added a commit that referenced this pull request Oct 17, 2016

crypto: naming anonymous functions
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

This comment has been minimized.

Member

MylesBorins commented Nov 11, 2016

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

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete
@sam-github

This comment has been minimized.

Member

sam-github commented Jul 25, 2017

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

This comment has been minimized.

Contributor

solebox commented Jul 27, 2017

thanks :)

sam-github added a commit that referenced this pull request Sep 20, 2017

@sam-github

This comment has been minimized.

Member

sam-github commented Sep 20, 2017

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

Landed in e9235e8 for v6.x-staging.

MylesBorins added a commit that referenced this pull request Sep 20, 2017

@MylesBorins MylesBorins referenced this pull request Sep 20, 2017

Merged

v6.11.4 proposal #15506

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment