FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

tools: eslint rule for missing internal/error doc by jasnell · Pull Request #16450 · nodejs/node · GitHub

/ node Public

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

Closed
jasnell wants to merge 2 commits into
nodejs:masterfrom
jasnell:errors-sync
ConversationCommits (2)ChecksFiles changed
Closed

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

Conversation

jasnell commented Oct 24, 2017

Copy link
Copy Markdown
Member
  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 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 approved these changes Oct 24, 2017
View reviewed changes

apapirovski left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

🥇 Love this!

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

cjihrig Oct 24, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Commented code

cjihrig commented Oct 24, 2017

Copy link
Copy Markdown
Contributor

cc: @not-an-aardvark

maclover7 reviewed Oct 24, 2017
View reviewed changes

maclover7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread doc/api/errors.md Outdated

maclover7 Oct 24, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

needs a comma after ever :)

Comment thread tools/eslint-rules/documented-errors.js Outdated

maclover7 Oct 24, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

✂️?

Comment thread test/parallel/test-eslint-documented-errors.js Outdated

maclover7 Oct 24, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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? 😬

richardlau Oct 24, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

not-an-aardvark Oct 24, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

maclover7 Oct 24, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

jasnell commented Oct 25, 2017

Copy link
Copy Markdown
Member Author

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

jasnell commented Oct 25, 2017

Copy link
Copy Markdown
Member Author

@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
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
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 commented Oct 26, 2017

Copy link
Copy Markdown
Member Author

Landed in fb477f3 and 76b8803

jasnell closed this Oct 26, 2017
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
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
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 mentioned this pull request Oct 30, 2017
Merged

gibfahn commented Oct 30, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

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
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
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

richardlau richardlau left review comments
cjihrig cjihrig approved these changes
+4 more reviewers
maclover7 maclover7 left review comments
not-an-aardvark not-an-aardvark approved these changes
apapirovski apapirovski approved these changes
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

Back | FazBrowse Home | New Git URL