Skip to content

tools: eslint rule for missing internal/error doc#16450

Closed
jasnell wants to merge 2 commits intonodejs:masterfrom
jasnell:errors-sync
Closed

tools: eslint rule for missing internal/error doc#16450
jasnell wants to merge 2 commits intonodejs:masterfrom
jasnell:errors-sync

Conversation

@jasnell
Copy link
Member

@jasnell jasnell commented Oct 24, 2017

  1. Adds a lint rule to catch undocumented internal/errors
  2. Adds docs for missing internal/error codes
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc, tools

@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. tools Issues and PRs related to the tools directory. labels Oct 24, 2017
apapirovski
apapirovski approved these changes Oct 24, 2017
View reviewed changes
Copy link
Contributor

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

🥇 Love this!

cjihrig
cjihrig approved these changes Oct 24, 2017
View reviewed changes
tools/eslint-rules/documented-errors.js Outdated
Copy link
Contributor

@cjihrig cjihrig Oct 24, 2017

Choose a reason for hiding this comment

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

Commented code

@cjihrig
Copy link
Contributor

cjihrig commented Oct 24, 2017

cc: @not-an-aardvark

maclover7
maclover7 reviewed Oct 24, 2017
View reviewed changes
Copy link
Contributor

@maclover7 maclover7 left a comment

Choose a reason for hiding this comment

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

This is really great!!

My only concern is that there might be errors that are documented, but don't exist anymore in lib/internal/errors.js (this had come up at least once or twice). I had made my own lintdoc.js to try and deal with this (I had meant to PR into nodejs/node at some point). Not a huge deal, just probably just something collaborators should keep in mind as things land.

doc/api/errors.md Outdated
Copy link
Contributor

@maclover7 maclover7 Oct 24, 2017

Choose a reason for hiding this comment

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

needs a comma after ever :)

tools/eslint-rules/documented-errors.js Outdated
Copy link
Contributor

@maclover7 maclover7 Oct 24, 2017

Choose a reason for hiding this comment

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

✂️?

test/parallel/test-eslint-documented-errors.js Outdated
Copy link
Contributor

@maclover7 maclover7 Oct 24, 2017

Choose a reason for hiding this comment

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

I could be missing something, but do we need to have this test file if we already have the lint rule, and it's enabled for lib/internal/errors.js? In theory the lint rule should run as part of make lint, right? 😬

Copy link
Member

@richardlau richardlau Oct 24, 2017

Choose a reason for hiding this comment

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

The test file tests the lint rule -- make lint runs it.

Copy link
Contributor

@not-an-aardvark not-an-aardvark Oct 24, 2017

Choose a reason for hiding this comment

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

This test file is intended to test the lint rule itself. For example, if the rule had a bug where it never reported any errors, we probably wouldn't notice it when running make lint.

Copy link
Contributor

@maclover7 maclover7 Oct 24, 2017

Choose a reason for hiding this comment

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

👍 sorry, my bad, I read RuleTester as running the rule.

@jasnell
Copy link
Member Author

jasnell commented Oct 25, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/10950/

@jasnell
Copy link
Member Author

jasnell commented Oct 25, 2017

@maclover7 ... regarding codes that are in errors.md that aren't used any more, let's tackle that problem separately.. perhaps by using @watilde's md lint tool once that lands.

jasnell added a commit that referenced this pull request Oct 26, 2017
@jasnell
doc: add missing error codes
fb477f3 
PR-URL: #16450
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
jasnell added a commit that referenced this pull request Oct 26, 2017
@jasnell
tools: add eslint rule for documented errors
76b8803 
PR-URL: #16450
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Oct 26, 2017

Landed in fb477f3 and 76b8803

@jasnell jasnell closed this Oct 26, 2017
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
@jasnell @addaleax
doc: add missing error codes
c1353b5 
PR-URL: nodejs/node#16450
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
@jasnell @addaleax
tools: add eslint rule for documented errors
f5883ed 
PR-URL: nodejs/node#16450
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
@gibfahn
Copy link
Member

gibfahn commented Oct 30, 2017 •
edited
Loading

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

I landed 76b8803 as it had no conflicts, so it's just fb477f3 that needs backporting. Obviously that fails linting 😁 , if someone could backport both that'd be ideal

addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@jasnell @addaleax
doc: add missing error codes
048947a 
PR-URL: nodejs/node#16450
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@jasnell @addaleax
tools: add eslint rule for documented errors
0afd386 
PR-URL: nodejs/node#16450
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@richardlau richardlau richardlau left review comments

@cjihrig cjihrig cjihrig approved these changes

+4 more reviewers

@maclover7 maclover7 maclover7 left review comments

@not-an-aardvark not-an-aardvark not-an-aardvark approved these changes

@apapirovski apapirovski apapirovski approved these changes

@refack refack refack approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

errors Issues and PRs related to JavaScript errors originated in Node.js core. tools Issues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@jasnell @cjihrig @gibfahn @apapirovski @refack @maclover7 @richardlau @not-an-aardvark @MylesBorins @nodejs-github-bot