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

joyent/node-verror#63 Do not call sprintf with one argument #73

Closed
wants to merge 3 commits into from

Conversation

joyent-automation
Copy link

#63 Do not call sprintf with one argument

Portions contributed by: David Pacheco [email protected]

This PR was migrated-from-gerrit, https://cr.joyent.us/#/c/6608/.
The raw archive of this CR is here.
See MANTA-4594 for info on Joyent Eng's migration from Gerrit.

CR discussion

Patch Set 1 code comments
lib/verror.js#400 @joyent-automation

warning: variable is declared but never referenced: options

lib/verror.js#413 @joyent-automation

warning: trailing_comma

@davepacheco commented at 2019-07-16T19:04:41

Uploaded patch set 3: Commit message was updated.

@trentm commented at 2019-10-24T17:18:34

Patch Set 4: Commit message was updated.

Portions contributed by: David Pacheco <[email protected]>
@trentm
Copy link
Contributor

trentm commented Oct 24, 2019

testing notes

This passes make test. It also passes the @Netflix/nerror "perror.test.js" file when I drop this "lib/verror.js" into it:

$ ./node_modules/.bin/mocha -R spec --full-trace --no-exit --no-timeouts test/perror.test.js


  perror
    ✓ should create a PError
    ✓ should not allow multiple arguments


  2 passing (6ms)

kusor
kusor previously approved these changes Oct 24, 2019
Copy link

@kusor kusor left a comment

Choose a reason for hiding this comment

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

LGTM. Maybe a little bit of confusion on the line 214 of the README where PError is included into a list right before mentioning that classes support printf but, still, I think it's clear at the beginning of the document

@trentm
Copy link
Contributor

trentm commented Oct 24, 2019

confusion on the line 214 of the README

Good point. I'll update that.

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.

3 participants