Consistent error messages in all modules by micnic · Pull Request #3374 · nodejs/node · GitHub

This commit fixes some error messages that are not consistent with some general rules which most of the error messages follow.

This is an updated PR for master branch, which is based on the PR #892 and the discussion in #1220

Can these be linted somehow? cc @Trott and @silverwind?

I think we would have to have some very specific, well defined rules in order to lint natural language. I'm not sure that we ever established rules in #892. This is definitely a step in the right direction IMO though.

I'm sure they can. How about something like this?

Start with uppercase or double quote.
Always end in a dot.
Quotes must be balanced.
Warn on ' and backticks.

Rule 2 is inconsistent in this PR, btw. But we could also omit the dot. Which style is prefered?

Warning on ' could be annoying, forcing everyone to not use contractions :-)

Can we include a period at the end of all messages? They are full sentences after all.

On the one hand, yes, they are complete sentences.

On the other hand, this is a typical usage:

try {
    // whatever
} catch (e) {
    console.error('Error "' + e.message +'" received while trying to foo.");
}

I can go either way, but agree that whatever is decided, it should be consistent.

@silverwind not all of them are sentences, for example bad argument https://github.com/nodejs/node/pull/3374/files#diff-9a205ef7ee967ee32efee02e58b3482dR1965

I would propose to go without period at the end as this rule can be applied to all cases

Fine with me. Go ahead and remove those periods.

Updated the PR to have no periods at the end of error messages.

LGTM. Are we fine with landing this as a patch? I don't suppose we count error messages as part of the API?

I think this conflicts with #3100.

I'd land this as a patch if no one objects, @micnic can we get a rebase?

This commit fixes some error messages that are not consistent with some general rules which most of the error messages follow

@silverwind done

This commit fixes some error messages that are not consistent with some general rules which most of the error messages follow. PR-URL: #3374 Reviewed-By: Roman Reiss <me@silverwind.io>

Thanks! Landed in 20285ad.

Was this landed without running CI? I'm seeing a number of tests failing that depend on the value of the error message.

Working on a fix

Looks like it, sorry about that.

I think this should be semver-minor. We're going to see a ton of tests in modules break because of these formatting changes, as we've seen in other releases based on similar changes in libuv.

We should also probably not take this in LTS for the same reason.

This could even be semver-major IMO. People could have tests or code that rely on these error messages...

We established some time back that "breaking tests" !== "breaking code/changes." The exact text of an error message is no guaranteed by the API, only that an error will exist (or throw). This is a perfect example, while some tests may break the code those tests are testing is not actually broken by the change.

Yeah, probably best to not have it in LTS. While I'd consider error code and error type to be part of the API, error messages are kind of a grey zone. Not sure it fits the minor label, but feel free to add it @mikeal.

Marked as minor, even thought it doesn't exactly qualify as a feature.

Tagged this and #3727 as semver-major as per #3776.

nodejs/node#3374

micnic mentioned this pull request Oct 14, 2015

Consistent error messages in all modules #892

Closed

silverwind reviewed Oct 15, 2015
View reviewed changes

silverwind mentioned this pull request Nov 9, 2015

crypto: improve crypto error messages #3100

Closed

Consistent error messages in all modules

1bd0c78

This commit fixes some error messages that are not consistent with some general rules which most of the error messages follow

silverwind closed this Nov 9, 2015

silverwind added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 10, 2015

cjihrig mentioned this pull request Nov 10, 2015

test: fix tests that check error messages #3727

Closed

Fishrock123 mentioned this pull request Nov 11, 2015

Rediscuss if error message changes are semver-major #3776

Closed

silverwind added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-minor PRs that contain new features and should be released in the next minor version. labels Nov 12, 2015

Fishrock123 mentioned this pull request Nov 26, 2015

deps: upgrade npm to 3.5.0 #4032

Closed

Fishrock123 mentioned this pull request Dec 15, 2015

v5.3.0 Release Proposal (Stable) #4281

Closed

rvagg mentioned this pull request Feb 9, 2016

buffer: validate list elements in Buffer.concat #4951

Merged

micnic mentioned this pull request Feb 17, 2016

Meta: new label - error-handling #5260

Closed

jasnell mentioned this pull request Mar 17, 2016

Planning for v6 #5766

Closed

ChALkeR mentioned this pull request May 2, 2016

API stability level «Locked» is inaccurate #6528

Closed

trentm mentioned this pull request Jun 28, 2016

node 6.x breakage in "restify-client tests GH-115 GET path with spaces" test case restify/clients#68

Closed

reconbot added a commit to serialport/node-serialport that referenced this pull request Nov 5, 2016

Conform to NodeJS error message formats

fe15480

nodejs/node#3374

reconbot mentioned this pull request Nov 5, 2016

Conform to NodeJS error message formats serialport/node-serialport#998

Merged

reconbot added a commit to serialport/node-serialport that referenced this pull request Nov 5, 2016

Conform to NodeJS error message formats (#998 )

672e831

nodejs/node#3374

ChALkeR mentioned this pull request Feb 8, 2017

Changing Definition of "Locked" for Stability Index in API Docs #11200

Closed

reconbot added a commit to serialport/utilities that referenced this pull request Mar 9, 2018

Conform to NodeJS error message formats (#998)

7787fed

nodejs/node#3374

reconbot added a commit to serialport/node-serialport that referenced this pull request Jul 24, 2018

Conform to NodeJS error message formats (#998 )

b1772d7

nodejs/node#3374

RafaelGSS mentioned this pull request Nov 11, 2023

lib: align console.table row to the left #50135

Merged

Consistent error messages in all modules by micnic · Pull Request #3374 · nodejs/node · GitHub

Uh oh!

Uh oh!

Consistent error messages in all modules - #3374

Consistent error messages in all modules#3374micnic wants to merge 1 commit intonodejs:masternodejs/node:masterfrom micnic:masterCopy head branch name to clipboard

Conversation

micnic commented Oct 14, 2015

Uh oh!

Fishrock123 commented Oct 14, 2015

Uh oh!

cjihrig commented Oct 14, 2015

Uh oh!

silverwind commented Oct 14, 2015

Uh oh!

mscdex commented Oct 14, 2015

Uh oh!

silverwind Oct 15, 2015

Choose a reason for hiding this comment

Uh oh!

Trott Oct 15, 2015

Choose a reason for hiding this comment

Uh oh!

micnic Oct 15, 2015

Choose a reason for hiding this comment

Uh oh!

silverwind Oct 15, 2015

Choose a reason for hiding this comment

Uh oh!

micnic commented Oct 19, 2015

Uh oh!

silverwind commented Oct 19, 2015

Uh oh!

silverwind commented Nov 9, 2015

Uh oh!

micnic commented Nov 9, 2015

Uh oh!

silverwind commented Nov 9, 2015

Uh oh!

cjihrig commented Nov 9, 2015

Uh oh!

cjihrig commented Nov 9, 2015

Uh oh!

silverwind commented Nov 9, 2015

Uh oh!

mikeal commented Nov 9, 2015

Uh oh!

evanlucas commented Nov 9, 2015

Uh oh!

mikeal commented Nov 9, 2015

Uh oh!

silverwind commented Nov 9, 2015

Uh oh!

silverwind commented Nov 10, 2015

Uh oh!

silverwind commented Nov 12, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Consistent error messages in all modules#3374
micnic wants to merge 1 commit into
nodejs:masterfrom
micnic:master