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

Rediscuss if error message changes are semver-major #3776

Closed
Fishrock123 opened this issue Nov 11, 2015 · 11 comments
Closed

Rediscuss if error message changes are semver-major #3776

Fishrock123 opened this issue Nov 11, 2015 · 11 comments
Labels
meta Issues and PRs related to the general management of the project.

Comments

@Fishrock123
Copy link
Contributor

See #3374

Opening here to tag for the ctc agenda.

@Fishrock123
Copy link
Contributor Author

Fwiw I haven't landed 20285ad in v5.x for this (v5.1.0) release because I'm a little less sure about it.

@cjihrig
Copy link
Contributor

cjihrig commented Nov 11, 2015

I think #3374 was a broad enough change to constitute a semver-major. We don't generally change error messages just for the sake of changing them. I also think that if an error message changes as the side effect of a bug fix, that shouldn't constitute a semver-major.

@silverwind
Copy link
Contributor

It's likely to have code like this in the wild:

if (err && /something/.test(err.message))
  // do something specific to that error

In this regard, one could see the message as part of the API. So it should be tagged as major or reverted, but generally I'm in support of consistency.

@silverwind
Copy link
Contributor

I tagged #3374 and #3727 as semver-major.

@silverwind
Copy link
Contributor

cc @micnic

@OrangeDog
Copy link

Checking error strings is bad practice, and I don't see that we should necessarily support it.
As well as minor changes, typos, improved explanations, error strings may also be localised.

If specific errors need specific handling, then error codes should be used instead.

@micnic
Copy link
Contributor

micnic commented Nov 12, 2015

I also agree that checking error messages is a bad practice, and I guess that in the wild it is somewhere used, so I think we should put it as a major change to minimize the frustration of those who check them when using a new version of node.

Maybe in the future we could add some prefixed error messages with a fixed code like this: [code: 0123] <Variable error message>

@OrangeDog
Copy link

Most errors already have a status code, an errno, an exit status, etc.

@Fishrock123
Copy link
Contributor Author

TSC CTC punted it a week or so. Not going to be in the v5.1.0 release though.

@Fishrock123 Fishrock123 added the meta Issues and PRs related to the general management of the project. label Jan 11, 2016
@Fishrock123
Copy link
Contributor Author

Related: #4311

@jasnell
Copy link
Member

jasnell commented Mar 22, 2016

Believe this can be closed now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

No branches or pull requests

6 participants