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

Add missing new for errors lib/*.js #1246

Closed
wants to merge 2 commits into from
Closed

Add missing new for errors lib/*.js #1246

wants to merge 2 commits into from

Conversation

nstepien
Copy link
Contributor

It doesn't break anything, but it does add an unwanted line in the stack trace.

@petkaantonov
Copy link
Contributor

LGTM

@mscdex mscdex added the crypto Issues and PRs related to the crypto subsystem. label Mar 24, 2015
@nstepien
Copy link
Contributor Author

Now that I look at it, there's a bunch of similar throw Error(...) code, maybe I should send a more complete PR?

@benjamingr
Copy link
Member

There should probably be an established policy of whether or not to include new before errors.

@petkaantonov
Copy link
Contributor

Seems a no-brainer to me, when not including new you potentially lose 1 useful frame and add 1 useless frame

Not including `new` adds a useless frame and removes a potentially useful frame.
@nstepien
Copy link
Contributor Author

Changed my commit to fix the other cases I could find in lib where errors were created without new.
PTAL.

@nstepien nstepien changed the title Add missing new for a TypeError in crypto.js Add missing new for errors lib/*.js Mar 24, 2015
@petkaantonov
Copy link
Contributor

changing all error constructor calls LGTM

@silverwind
Copy link
Contributor

There should probably be an established policy of whether or not to include new before errors.

I think instead of writing down every little rule, we should just enforce these through a linter.

@silverwind
Copy link
Contributor

Also, LGTM

@Fishrock123 Fishrock123 removed the crypto Issues and PRs related to the crypto subsystem. label Mar 24, 2015
@@ -108,12 +108,12 @@ exports.lookup = function lookup(hostname, options, callback) {

// Parse arguments
if (hostname && typeof hostname !== 'string') {
throw TypeError('invalid arguments: hostname must be a string or falsey');
throw new TypeError('invalid arguments: hostname must be a string or falsey');
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is now 82 characters long, could you wrap it?

@brendanashworth
Copy link
Contributor

One comment, otherwise LGTM.

@nstepien
Copy link
Contributor Author

@brendanashworth
There you go.

brendanashworth pushed a commit that referenced this pull request Mar 24, 2015
Not including `new` adds a useless frame and removes a potentially
useful frame.

PR-URL: #1246
Reviewed-By: Petka Antonov <petka_antonov@hotmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
@brendanashworth
Copy link
Contributor

Thanks @MayhemYDG, this has been merged in 1832743!

@nstepien nstepien deleted the patch-1 branch March 24, 2015 19:45
@silverwind
Copy link
Contributor

Noticed this commit got merged with a nickname instead of a real name as git user.name, which isn't ideal, especially when it comes to AUTHORS.

It's too late now to fix that, and everyone who LGTM'd that is probably guilty, including me 😇.

Just something to keep an look out in the future @iojs/collaborators.

@nstepien
Copy link
Contributor Author

@silverwind
Mmh, to my defense CONTRIBUTING.md isn't 100% clear on that one.

$ git config --global user.name "J. Random User"

"Random User" sounds like a nickname.

Either way you got my name in my email address.

@silverwind
Copy link
Contributor

Yeah, that's partially at fault too. I'll mention it on the next AUTHORS update.

@tellnes
Copy link
Contributor

tellnes commented Mar 27, 2015

This also changes lib/punycode.js which is maintained externally.

mathiasbynens pushed a commit to mathiasbynens/punycode.js that referenced this pull request Mar 27, 2015
Although it is optional, when not including `new` you potentially lose a single useful frame in the stack trace and add a useless frame.

nodejs/node#1246

Closes #32.
@rlidwka
Copy link
Contributor

rlidwka commented Mar 29, 2015

Maybe it's better to report this to v8 team, so they could make new Error() and Error() behaviors identical?

benjamingr added a commit to benjamingr/io.js that referenced this pull request Mar 27, 2016
Update punycode to the latest released version. This is mainly in
order to further reduce the maintenance burden. In
nodejs#1246 a fix introducing `new` to
errors was introduced and it has since been ported back to the
punycode library.

This puts Node back in sync with the library itself so it can receive
future fixes and updates directly.

PR-URL:
Reviewed-By:
Reviewed-By:
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants