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

Consistent error messages in all modules #3374

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
8 participants
@micnic
Copy link
Contributor

commented Oct 14, 2015

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

@Fishrock123

This comment has been minimized.

Copy link
Member

commented Oct 14, 2015

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

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2015

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.

@silverwind

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2015

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?

@mscdex

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2015

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

@silverwind

View changes

lib/_http_outgoing.js Outdated
@@ -92,7 +92,7 @@ OutgoingMessage.prototype.setTimeout = function(msecs, callback) {

if (callback) {
if (typeof callback !== 'function')
throw new TypeError('callback must be a function');
throw new TypeError('"callback" must be a function');

This comment has been minimized.

Copy link
@silverwind

silverwind Oct 15, 2015

Contributor

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

This comment has been minimized.

Copy link
@Trott

Trott Oct 15, 2015

Member

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.

This comment has been minimized.

Copy link
@micnic

micnic Oct 15, 2015

Author Contributor

@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

This comment has been minimized.

Copy link
@silverwind

silverwind Oct 15, 2015

Contributor

Fine with me. Go ahead and remove those periods.

@micnic

This comment has been minimized.

Copy link
Contributor Author

commented Oct 19, 2015

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

@silverwind

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2015

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

@silverwind

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2015

I think this conflicts with #3100.

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

Consistent error messages in all modules
This commit fixes some error messages that are not consistent with
some general rules which most of the error messages follow
@micnic

This comment has been minimized.

Copy link
Contributor Author

commented Nov 9, 2015

micnic added a commit that referenced this pull request Nov 9, 2015

lib: Consistent error messages in all modules
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>
@silverwind

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2015

Thanks! Landed in 20285ad.

@silverwind silverwind closed this Nov 9, 2015

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2015

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

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2015

Working on a fix

@silverwind

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2015

Looks like it, sorry about that.

@mikeal

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2015

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.

@evanlucas

This comment has been minimized.

Copy link
Member

commented Nov 9, 2015

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

@mikeal

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2015

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.

@silverwind

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2015

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.

@silverwind

This comment has been minimized.

Copy link
Contributor

commented Nov 10, 2015

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

@silverwind

This comment has been minimized.

Copy link
Contributor

commented Nov 12, 2015

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

@jasnell jasnell referenced this pull request Mar 17, 2016

Closed

Planning for v6 #5766

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

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

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.