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

doc: error message are still major #14375

Merged
merged 1 commit into from
Jul 21, 2017

Conversation

refack
Copy link
Contributor

@refack refack commented Jul 19, 2017

Ratifying the decision from #13937

Checklist
Affected core subsystem(s)

doc,error

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jul 19, 2017
@refack refack requested a review from jasnell July 19, 2017 19:37
@refack refack added errors Issues and PRs related to JavaScript errors originated in Node.js core. meta Issues and PRs related to the general management of the project. labels Jul 19, 2017
changes to `internal/errors` error messages will be handled as `semver-minor`
or `semver-patch`.
considered a `semver-major` change. In the near future after the transition
has been completed (but no sooner then `v10`) error message changes will be
Copy link
Member

Choose a reason for hiding this comment

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

typos: than, and commas after future and )

also, I’d prefer Node 10.x over v10

Copy link
Member

Choose a reason for hiding this comment

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

I don't believe we can say "no sooner" than v10. The decision was to handle the changes as semver-major in 8.x and to revisit within 9.x... which means we could start handling them as semver-minor as early as 9.0.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack X 2

considered a `semver-major` change. However, once using `internal/errors`,
changes to `internal/errors` error messages will be handled as `semver-minor`
or `semver-patch`.
considered a `semver-major` change. In the near future, after the transition
Copy link
Member

Choose a reason for hiding this comment

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

Nit: remove near

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

changes to `internal/errors` error messages will be handled as `semver-minor`
or `semver-patch`.
considered a `semver-major` change. In the near future, after the transition
has been completed, error message changes will be handled as `semver-minor` or
Copy link
Member

Choose a reason for hiding this comment

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

Nit: will -> may

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • will hopefully? (if we're optimistic)
  • might? (if we're skeptic)
  • or even error message changes will be reevaluated, with the goal being to handle then as (if we want to explain)

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Left some nits, but with or without 'em, LGTM.

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

LGTM with @Trott's nits addressed.

changes to `internal/errors` error messages will be handled as `semver-minor`
or `semver-patch`.
considered a `semver-major` change. In the future, after the transition
has been completed, error message changes will be handled as `semver-minor` or
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still bikeshedding over will: #14375 (review)
image

Copy link
Contributor

Choose a reason for hiding this comment

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

what about should? That gives us the opportunity to change if we absolutely have to, but shows that is the direction we are trying to go?

Copy link
Member

Choose a reason for hiding this comment

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

I am pro should or your third suggestion @refack.

@refack
Copy link
Contributor Author

refack commented Jul 20, 2017

Went with should:

In the future, after the transition has been completed, error message changes should be handled
as `semver-minor` or `semver-patch`.

considered a `semver-major` change. However, once using `internal/errors`,
changes to `internal/errors` error messages will be handled as `semver-minor`
considered a `semver-major` change. In the future, after the transition
has been completed, error message changes should be handled as `semver-minor`
Copy link
Member

Choose a reason for hiding this comment

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

Nit: This makes it sound like once the errors have been all migrated, the change to treating them as not-breaking is automatic. Honestly, I think the best/easiest thing to do at this point is to remove the sentence. It's not needed. It's about a possible future state that is likely to come (but not guaranteed) and we certainly haven't agreed as to exactly when it will come. So just leave it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense.
Originally I thought we should end with a "hopeful" note. But that's sort of the subject of the guide as a whole.

@refack
Copy link
Contributor Author

refack commented Jul 20, 2017

To all who approved, current status is that the sentence dealing with future plans was dropped. PTAL.

@Trott
Copy link
Member

Trott commented Jul 20, 2017

Still LGTM.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

considered a `semver-major` change. However, once using `internal/errors`,
changes to `internal/errors` error messages will be handled as `semver-minor`
or `semver-patch`.
considered a `semver-major` change.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is 100% true, if the error message remains the same?

Copy link
Contributor Author

@refack refack Jul 21, 2017

Choose a reason for hiding this comment

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

The Error's name changes to ${super.name} [${this[kCode]}] so the .toString() changes.

PR-URL: nodejs#14375
Refs: nodejs#13937
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@refack refack merged commit ac81267 into nodejs:master Jul 21, 2017
addaleax pushed a commit that referenced this pull request Jul 22, 2017
PR-URL: #14375
Refs: #13937
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Jul 24, 2017
PR-URL: #14375
Refs: #13937
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@addaleax addaleax mentioned this pull request Jul 24, 2017
@addaleax addaleax mentioned this pull request Aug 2, 2017
@refack refack deleted the semver-major-error-still branch September 4, 2017 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. errors Issues and PRs related to JavaScript errors originated in Node.js core. meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.