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

errors: consistent format for error messages #16904

Closed

Conversation

apapirovski
Copy link
Member

Consistently use printf-style strings for error messages that do not need a custom argument order or processing of arguments.

Also added an eslint rule (with a test) that checks for this and warns about it.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

errors, test, tools

Consistently use printf-style strings for error messages that
do not need a custom argument order or processing of arguments.
@apapirovski apapirovski added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Nov 9, 2017
@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Nov 9, 2017
@apapirovski
Copy link
Member Author

Copy link
Contributor

@maclover7 maclover7 left a comment

Choose a reason for hiding this comment

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

Personally, I like literals better, but as long as there's one standard that's enforced by the linter, fine by me 😄

create: function(context) {
return {
ExpressionStatement: function(node) {
if (!isDefiningError(node))
Copy link
Contributor

Choose a reason for hiding this comment

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

I've been meaning to do this, but while you're here, can you extract this to rules-utils.js, and update the other usage of this?

@apapirovski
Copy link
Member Author

@maclover7 Yeah, I'm fine with either. We currently have more printf-formatted errors so this made sense to me (and the fact that it avoids creating all these extra functions) but either is fine.

Copy link
Member

@mhdawson mhdawson 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

Landed in 4b82d89

@addaleax addaleax closed this Nov 18, 2017
addaleax pushed a commit that referenced this pull request Nov 18, 2017
Consistently use printf-style strings for error messages that
do not need a custom argument order or processing of arguments.

PR-URL: #16904
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@apapirovski apapirovski deleted the patch-consistent-error-format branch November 18, 2017 19:35
@MylesBorins
Copy link
Contributor

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

apapirovski added a commit to apapirovski/node that referenced this pull request Dec 11, 2017
Consistently use printf-style strings for error messages that
do not need a custom argument order or processing of arguments.

PR-URL: nodejs#16904
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@apapirovski
Copy link
Member Author

Backport in #17624

MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
Consistently use printf-style strings for error messages that
do not need a custom argument order or processing of arguments.

Backport-PR-URL: #17624
PR-URL: #16904
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
Consistently use printf-style strings for error messages that
do not need a custom argument order or processing of arguments.

Backport-PR-URL: #17624
PR-URL: #16904
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
Consistently use printf-style strings for error messages that
do not need a custom argument order or processing of arguments.

Backport-PR-URL: #17624
PR-URL: #16904
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
@gibfahn
Copy link
Member

gibfahn commented Dec 19, 2017

Should this be backported to v6.x and v8.x-staging? If so please include #17376.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants