GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Consistent error messages in all modules by micnic · Pull Request #3374 · nodejs/node

Conversation

@micnic

@micnic micnic commented Oct 14, 2015

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@Fishrock123

Fishrock123 commented Oct 14, 2015

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@micnic micnic mentioned this pull request Oct 14, 2015
Consistent error messages in all modules #892
Closed
@cjihrig

cjihrig commented Oct 14, 2015

Copy link
Copy Markdown
Contributor

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@silverwind

silverwind commented Oct 14, 2015

Copy link
Copy Markdown
Contributor

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

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mscdex

mscdex commented Oct 14, 2015

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
silverwind
silverwind reviewed Oct 15, 2015
View reviewed changes
Comment thread lib/_http_outgoing.js Outdated

@silverwind silverwind Oct 15, 2015

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@Trott Trott Oct 15, 2015

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@micnic micnic Oct 15, 2015

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@silverwind silverwind Oct 15, 2015

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Fine with me. Go ahead and remove those periods.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@micnic

micnic commented Oct 19, 2015

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@silverwind

silverwind commented Oct 19, 2015

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@silverwind silverwind mentioned this pull request Nov 9, 2015
crypto: improve crypto error messages #3100
Closed
@silverwind

silverwind commented Nov 9, 2015

Copy link
Copy Markdown
Contributor

I think this conflicts with #3100.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@micnic
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
@micnic

micnic commented Nov 9, 2015

Copy link
Copy Markdown
Contributor Author

@silverwind done
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
micnic added a commit that referenced this pull request Nov 9, 2015
@micnic @silverwind
lib: Consistent error messages in all modules
20285ad 
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 <[email protected]>
@silverwind

silverwind commented Nov 9, 2015

Copy link
Copy Markdown
Contributor

Thanks! Landed in 20285ad.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@silverwind silverwind closed this Nov 9, 2015
@cjihrig

cjihrig commented Nov 9, 2015

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig

cjihrig commented Nov 9, 2015

Copy link
Copy Markdown
Contributor

Working on a fix
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@silverwind

silverwind commented Nov 9, 2015

Copy link
Copy Markdown
Contributor

Looks like it, sorry about that.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mikeal

mikeal commented Nov 9, 2015

Copy link
Copy Markdown
Contributor

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@evanlucas

evanlucas commented Nov 9, 2015

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mikeal

mikeal commented Nov 9, 2015

Copy link
Copy Markdown
Contributor

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@silverwind

silverwind commented Nov 9, 2015

Copy link
Copy Markdown
Contributor

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@silverwind silverwind added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 10, 2015
@silverwind

silverwind commented Nov 10, 2015

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig cjihrig mentioned this pull request Nov 10, 2015
test: fix tests that check error messages #3727
Closed
@Fishrock123 Fishrock123 mentioned this pull request Nov 11, 2015
Rediscuss if error message changes are semver-major #3776
Closed
@silverwind 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
@silverwind

silverwind commented Nov 12, 2015

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Fishrock123 Fishrock123 mentioned this pull request Nov 26, 2015
deps: upgrade npm to 3.5.0 #4032
Closed
@Fishrock123 Fishrock123 mentioned this pull request Dec 15, 2015
v5.3.0 Release Proposal (Stable) #4281
Closed
@rvagg rvagg mentioned this pull request Feb 9, 2016
buffer: validate list elements in Buffer.concat #4951
Merged
@micnic micnic mentioned this pull request Feb 17, 2016
Meta: new label - error-handling #5260
Closed
@jasnell jasnell mentioned this pull request Mar 17, 2016
Planning for v6 #5766
Closed
@ChALkeR ChALkeR mentioned this pull request May 2, 2016
API stability level «Locked» is inaccurate #6528
Closed
@trentm 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
@reconbot
Conform to NodeJS error message formats
fe15480 
nodejs/node#3374
@reconbot 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
@reconbot
Conform to NodeJS error message formats (#998)
672e831 
nodejs/node#3374
@ChALkeR 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
@reconbot
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
@reconbot
Conform to NodeJS error message formats (#998)
b1772d7 
nodejs/node#3374
@RafaelGSS RafaelGSS mentioned this pull request Nov 11, 2023
lib: align console.table row to the left #50135
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@micnic @Fishrock123 @cjihrig @silverwind @mscdex @mikeal @evanlucas @Trott
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.