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

jasnell · 2017-10-24T18:18:29Z

Adds a lint rule to catch undocumented internal/errors
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

apapirovski

🥇 Love this!

cjihrig · 2017-10-24T18:33:22Z

tools/eslint-rules/documented-errors.js

@@ -0,0 +1,49 @@
+'use strict';
+
+// const prefix = 'Out of ASCIIbetical order - ';


Commented code

cjihrig · 2017-10-24T18:33:53Z

cc: @not-an-aardvark

maclover7

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.

maclover7 · 2017-10-24T18:28:32Z

doc/api/errors.md

+### ERR_CRYPTO_HASH_UPDATE_FAILED
+
+Used when [`hash.update()`][] fails for any reason. This should rarely, if
+ever happen.


needs a comma after ever :)

maclover7 · 2017-10-24T18:30:50Z

tools/eslint-rules/documented-errors.js

@@ -0,0 +1,49 @@
+'use strict';
+
+// const prefix = 'Out of ASCIIbetical order - ';


maclover7 · 2017-10-24T18:32:30Z

test/parallel/test-eslint-documented-errors.js

+
+const invalidCode = 'UNDOCUMENTED ERROR CODE';
+
+new RuleTester().run('documented-errors', rule, {


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

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

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.

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

jasnell · 2017-10-25T04:41:54Z

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

jasnell · 2017-10-25T18:47:01Z

@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.

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 · 2017-10-26T15:00:50Z

Landed in fb477f3 and 76b8803

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>

gibfahn · 2017-10-30T20:16:19Z

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

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>

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

refack approved these changes Oct 24, 2017

View reviewed changes

cjihrig approved these changes Oct 24, 2017

View reviewed changes

maclover7 reviewed Oct 24, 2017

View reviewed changes

not-an-aardvark approved these changes Oct 24, 2017

View reviewed changes

jasnell force-pushed the errors-sync branch from 97716fb to 3620f1f Compare October 25, 2017 18:45

jasnell added 2 commits October 26, 2017 07:55

doc: add missing error codes

a549dd1

tools: add eslint rule for documented errors

30a79b5

jasnell force-pushed the errors-sync branch from 3620f1f to 30a79b5 Compare October 26, 2017 14:57

jasnell closed this Oct 26, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

gibfahn added the backport-requested-v8.x label Oct 30, 2017

MylesBorins added the dont-land-on-v6.x label Nov 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jasnell commented Oct 24, 2017

apapirovski left a comment

cjihrig Oct 24, 2017

cjihrig commented Oct 24, 2017

maclover7 left a comment

maclover7 Oct 24, 2017

maclover7 Oct 24, 2017

maclover7 Oct 24, 2017

richardlau Oct 24, 2017

not-an-aardvark Oct 24, 2017

maclover7 Oct 24, 2017

jasnell commented Oct 25, 2017

jasnell commented Oct 25, 2017

jasnell commented Oct 26, 2017

gibfahn commented Oct 30, 2017 •

edited

Loading

		@@ -0,0 +1,49 @@
		'use strict';

		// const prefix = 'Out of ASCIIbetical order - ';


		const invalidCode = 'UNDOCUMENTED ERROR CODE';

		new RuleTester().run('documented-errors', rule, {

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

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

Conversation

jasnell commented Oct 24, 2017

Checklist

Affected core subsystem(s)

apapirovski left a comment

Choose a reason for hiding this comment

cjihrig Oct 24, 2017

Choose a reason for hiding this comment

cjihrig commented Oct 24, 2017

maclover7 left a comment

Choose a reason for hiding this comment

maclover7 Oct 24, 2017

Choose a reason for hiding this comment

maclover7 Oct 24, 2017

Choose a reason for hiding this comment

maclover7 Oct 24, 2017

Choose a reason for hiding this comment

richardlau Oct 24, 2017

Choose a reason for hiding this comment

not-an-aardvark Oct 24, 2017

Choose a reason for hiding this comment

maclover7 Oct 24, 2017

Choose a reason for hiding this comment

jasnell commented Oct 25, 2017

jasnell commented Oct 25, 2017

jasnell commented Oct 26, 2017

gibfahn commented Oct 30, 2017 • edited Loading

gibfahn commented Oct 30, 2017 •

edited

Loading