FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

errors: consistent format for error messages by apapirovski · Pull Request #16904 · nodejs/node · GitHub

/ node Public

errors: consistent format for error messages - #16904

Closed
apapirovski wants to merge 1 commit into
nodejs:masterfrom
apapirovski:patch-consistent-error-format
ConversationCommits (1)ChecksFiles changed
Closed

errors: consistent format for error messages#16904
apapirovski wants to merge 1 commit into
nodejs:masterfrom
apapirovski:patch-consistent-error-format

Conversation

apapirovski commented Nov 9, 2017

Copy link
Copy Markdown
Contributor

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

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

apapirovski commented Nov 9, 2017

Copy link
Copy Markdown
Contributor Author

CI: https://ci.nodejs.org/job/node-test-pull-request/11326/

maclover7 reviewed Nov 9, 2017
View reviewed changes

maclover7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread tools/eslint-rules/prefer-util-format-errors.js
create: function(context) {
return {
ExpressionStatement: function(node) {
if (!isDefiningError(node))

maclover7 Nov 9, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Nov 9, 2017

Copy link
Copy Markdown
Contributor 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.

mhdawson approved these changes Nov 16, 2017
View reviewed changes

mhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

addaleax commented Nov 18, 2017

Copy link
Copy Markdown
Member

Landed in 4b82d89

addaleax closed this Nov 18, 2017
addaleax pushed a commit that referenced this pull request Nov 18, 2017
errors: consistent format for error message
4b82d89 
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 deleted the patch-consistent-error-format branch November 18, 2017 19:35

MylesBorins commented Dec 11, 2017

Copy link
Copy Markdown
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
errors: consistent format for error message
4aff4e8 
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 mentioned this pull request Dec 11, 2017
Closed
3 tasks

apapirovski commented Dec 12, 2017

Copy link
Copy Markdown
Contributor Author

Backport in #17624

MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
errors: consistent format for error message
c1a2e57 
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
errors: consistent format for error message
d1d2aa9 
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
errors: consistent format for error message
a333e71 
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 mentioned this pull request Dec 12, 2017
Merged

gibfahn commented Dec 19, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

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

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
addaleax addaleax approved these changes
+2 more reviewers
maclover7 maclover7 left review comments
mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL