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

errors: documenting removed error codes #22061

Closed
2 tasks
joyeecheung opened this issue Aug 1, 2018 · 12 comments
Closed
2 tasks

errors: documenting removed error codes #22061

joyeecheung opened this issue Aug 1, 2018 · 12 comments
Labels
doc Issues and PRs related to the documentations. errors Issues and PRs related to JavaScript errors originated in Node.js core. help wanted Issues that need assistance from volunteers or PRs that need help to proceed.

Comments

@joyeecheung
Copy link
Member

See #21491 for a full list of removed/inconsistent error codes (h/t to @ChALkeR for the investigation). There could be more though

Actions requested

  • Add a new section in doc/api/errors.md documenting the removed error codes
  • Add change logs for those codes (releases in which they initially appeared and got removed). This requires a bit more archeology.
    • To find out when a code got added, look into the git blame of doc/api/errors.md (before it got removed) and identify the commit where it was added (GitHub's commit UI will show the tags where the commit appears so it shouldn't be too hard).
    • To find out when a code got added, see doc: remove 2 unused error codes from errors.md #21491
@ChALkeR ChALkeR added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Aug 1, 2018
@joyeecheung joyeecheung added help wanted Issues that need assistance from volunteers or PRs that need help to proceed. doc Issues and PRs related to the documentations. labels Aug 1, 2018
@SirR4T
Copy link
Contributor

SirR4T commented Aug 2, 2018

Hi @joyeecheung , @ChALkeR , willing to work on this, but need some additional pointers.

For instance, I can see that the ERR_FS_WATCHER... errors were added in commit 6c25f2e , but am unable to find the nodejs version tagged to this commit.

Is there any easier / programmatic way to find the nodejs release version, given a commit hash?

@joyeecheung
Copy link
Member Author

@SirR4T Thanks, you can find that out by looking at the GitHub UI of a commit, it shows the tags where the commit is present

screen shot 2018-08-02 at 7 46 02 pm

@SirR4T
Copy link
Contributor

SirR4T commented Aug 2, 2018

Oh. I guess i was on the right track then. Specifically for the ERR_FS_WATCHER... errors, the docs are going to look like added: v10.8.0 removed: v10.8.0, then? Are we ok with that?

I guess it should look like added: v10.0.0 removed: v10.8.0. Correct?

@joyeecheung
Copy link
Member Author

@SirR4T I think it was added in v10.0.0 given the UI, but it was not removed in v10.8.0 - even the code is removed, the commit will still be there because we do not remove commits. It was removed by another commit on top of that and the job is to find out what that commit is.

@SirR4T
Copy link
Contributor

SirR4T commented Aug 2, 2018

Oh i meant the documentation in doc/api/errors.md, where I'm assuming this will be a new section (say Legacy Node.js Error Codes, a subsection after Node.js Error Codes)

As I understand, the point of this PR was to document lifecycle of the Error Codes which were removed. Do let me know if i'm getting it wrong?

@BridgeAR
Copy link
Member

BridgeAR commented Aug 2, 2018

@joyeecheung I was originally in favor of having the removed error codes in a separate list but after giving it another thought I actually believe that is not necessary. The reason is that google should know about the old error codes in the correct Node.js version. The user who looks at the docs should always check the specific docs version that matches the used version and in those cases the error codes would be documented.

So I wonder if we really need that list as it is manual work to keep it aligned and there is probably little benefit having it.

@SirR4T
Copy link
Contributor

SirR4T commented Aug 2, 2018

Oh btw, in answer to

Is there any easier / programmatic way to find the nodejs release version, given a commit hash?

, I found this works (depends on the excellent pup tool):

$ curl -s https://github.com/nodejs/node/branch_commits/6c25f2ea49c2521dfd2423bf3a06222633ec4dc9 | pup 'body ul[class~="branches-tag-list"] li:not([class]) a text{}'
v10.8.0
v10.0.0

Could be helpful in fetching the tags for all the commit hashes. Still not sure if these answers are correct, though.

@joyeecheung
Copy link
Member Author

@SirR4T

I guess it should look like added: v10.0.0 removed: v10.8.0. Correct?

You can find out when the error was added in v10.0.0 only because it is correct to assume that an error is added along with that commit. But it's incorrect to assume that if the commit appears in a release the error will also in that release (= not removed). Say commit A added a code and a commit B later removed that, A will continue to appear in releases that also contain B even though the error has already been removed by B. You are seeing v10.8.0 only because v10.8.0 is the latest commit, when v10.9.0 comes out, the commit that added the code will also appear in v10.9.0, because commits cannot be removed (code can, in other commits).

@joyeecheung
Copy link
Member Author

joyeecheung commented Aug 2, 2018

The reason is that google should know about the old error codes in the correct Node.js version. The user who looks at the docs should always check the specific docs version that matches the used version and in those cases the error codes would be documented.

@BridgeAR I don't think we can count on Google (or our SEO) for that, try googling the errors that got removed and Google will likely give you the PR that touched the error but our docs, no matter which version, is unlikely to show up in the first page (or any page). Also making the error codes harder to discover by hiding the removed ones in older versions of docs may just discourage the usage of them. We have been advertising them as "static, permanent identifiers" and now there does not seem to be anything "static, permanent" about them given that they can be removed and changed without leaving a trace in the latest version of docs.

@targos
Copy link
Member

targos commented Aug 2, 2018

I think there is another good reason for keeping the list of old error codes: avoid reusing them for new errors.

@BridgeAR
Copy link
Member

BridgeAR commented Aug 2, 2018

It will definitely not hurt and not to reuse them is indeed important. So +1 on that.

@SirR4T
Copy link
Contributor

SirR4T commented Aug 3, 2018

Raised a work in progress PR, so that early reviews might help set the template for all error codes. Please let me know if this looks like what is required?

addaleax pushed a commit that referenced this issue Aug 27, 2018
PR-URL: #22100
Fixes: #22061
Refs: #21491
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
targos pushed a commit that referenced this issue Sep 3, 2018
PR-URL: #22100
Fixes: #22061
Refs: #21491
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
targos pushed a commit that referenced this issue Sep 6, 2018
PR-URL: #22100
Fixes: #22061
Refs: #21491
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
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. help wanted Issues that need assistance from volunteers or PRs that need help to proceed.
Projects
None yet
Development

No branches or pull requests

5 participants