doc, tools: use eslint-plugin-markdown by vsemozhetbyt · Pull Request #12563 · nodejs/node

vsemozhetbyt · 2017-04-21T05:10:34Z

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc, tools

Refs: #12557 (comment)

Commit 1: an initial step to eliminate parsing errors. Fixing strategies:

Mixed conflicting contexts: split into separate code blocks.
Object literals erroneously parsed as code blocks:
- add const defaults = or const options =
- ~~wrap in ()~~ (See doc, tools: use eslint-plugin-markdown #12563 (comment))
Terminal / REPL interaction logs in repl.md: add  to disable linting + preserve syntax highlighting. (see doc, tools: use eslint-plugin-markdown #12563 (comment))
Non-js outputs: wrap in ``` ``` instead of ```js ```.
Truncated constructions: complete the code.
Uncommented ellipses; comment out.
Syntax errors: fix.
Line numbers in front of the code: move to the line ends and comment out.

Commit 2: conform most of the problematic code to the rules.

Code is checked with this doc/.eslintrc.yaml (see https://gist.github.com/not-an-aardvark/f3cb021e854414128d197dde8d0f62b2)

plugins: [markdown]

rules:
  strict: 0
  no-restricted-properties: 0
  no-undef: 0
  no-unused-vars: 0

Commit 3: add eslint-plugin-markdown stuff

npm install eslint-plugin-markdown.
Add doc/.eslintrc.yaml; add plugins: [markdown] to the main .eslintrc.yaml.
Add  or  for the rest problematic code.
.js files in doc folder added to .eslintignore.
Update Makefile and vcbuild.bat.

Commit 4: make doc linting a bit more specific (as we do for tests code)

Add no-var: 2 and prefer-const: 2 in doc/.eslintrc.yaml.
Fix some fragments in docs accordingly.

not-an-aardvark · 2017-04-21T05:22:49Z

In terms of keeping a clean git commit history, I think it might be better to fix style/syntax in the docs in one commit (without adding directives like eslint-disable), and then add eslint-plugin-markdown and eslint-disable directives at the same time. Otherwise, the codebase will be in an intermediate state after this commit where there are eslint-disable directives that aren't actually used by anything.

vsemozhetbyt · 2017-04-21T05:43:23Z

@not-an-aardvark I've reverted  part for repl.md for now.

vsemozhetbyt · 2017-04-21T06:10:37Z

@not-an-aardvark I've checked the whole doc directory now, not only doc/api. Two fixes for guides added.

thefourtheye · 2017-04-21T06:21:17Z

doc/api/dns.md

ESLint parses this as a code block with statements and breaks with parsing errors. With () it is parsed as an expression with an object literal.

This doesn't feel right to me... If everybody else is okay with this, then fine.

In some places, I've added something like const defaults = { };, but it seems to me this does not fit everywhere. Maybe, somebody can think up a better approach.

I think readability should be our main concern in the docs. If a code block which isn't technically valid JS communicates more clearly than the valid-JS alternative, then we should keep using invalid JS and put a disable comment above it. In this case, I don't have a strong opinion on whether the parentheses increase readability.

vsemozhetbyt · 2017-04-21T18:37:58Z

@not-an-aardvark, @thefourtheye

I've reverted wrapping objects in ().
Most of the rule violations addressed in the second commit (the first post is updated). Currently, we have only these ones for  notes:

ESLint utput after the second commit:

doc\api\assert.md
  559:27  error  Unexpected string as second argument  assert-throws-arguments

doc\api\debugger.md
  33:3  error  Unexpected 'debugger' statement  no-debugger

doc\api\dns.md
  299:10  error  Parsing error: Unexpected token :
  358:13  error  Parsing error: Unexpected token :
  388:9   error  Parsing error: Unexpected token :

doc\api\http.md
    16:19  error  Parsing error: Unexpected token :
    43:20  error  Missing semicolon                  semi
  1464:20  error  Missing semicolon                  semi

doc\api\modules.md
  560:18  error  Function name `Constructor` should match variable name `exports`  func-name-matching

doc\api\os.md
  164:2  error  Missing semicolon                  semi
  274:7  error  Parsing error: Unexpected token :

doc\api\process.md
   751:8   error  Parsing error: Unexpected token :
   836:14  error  Missing semicolon                  semi
   842:50  error  Missing semicolon                  semi
   858:22  error  Missing semicolon                  semi
  1179:12  error  Parsing error: Unexpected token :
  1350:6   error  Parsing error: Unexpected token :
  1711:7   error  Parsing error: Unexpected token :

doc\api\querystring.md
  65:6  error  Parsing error: Unexpected token :

doc\api\repl.md
   44:1  error  Parsing error: Unexpected token >
   79:1  error  Parsing error: Unexpected token >
  107:3  error  Parsing error: Unexpected token node
  136:1  error  Parsing error: Unexpected token >
  146:1  error  Parsing error: Unexpected token >
  292:4  error  Parsing error: Unexpected token /
  442:3  error  Parsing error: Unexpected token node

doc\api\v8.md
  122:29  error  Parsing error: Unexpected token :

✖ 27 problems (27 errors, 0 warnings)

vsemozhetbyt · 2017-04-21T18:43:25Z

@not-an-aardvark What are the steps for the proceeding? Are these right?

run npm install eslint-plugin-markdown in the tools/eslint
Add doc/.eslintrc.yaml from the first post.
Add  notes.
Update configs for build and CI — I am afraid I will be no good for this.

not-an-aardvark · 2017-04-21T19:12:18Z

Yes, I think those are the right steps. If doc/api/addons.md is modified, I think we should run full CI since that is used to autogenerate something IIRC.

vsemozhetbyt · 2017-04-21T20:05:56Z

@not-an-aardvark
Plugin, doc/.eslintrc.yaml and directives are added in the third commit.

What should I add in build configs so that doc are checked in build and CI?

vsemozhetbyt · 2017-04-21T20:10:42Z

We also should ignore .js files in the doc/api_assets, they are full of linting errors.

not-an-aardvark · 2017-04-21T20:12:46Z

cc @Trott: I'm not sure about the best place to put eslint-plugin-markdown. ESLint needs to be able to call require('eslint-plugin-markdown') to get the module, and this would normally work fine if eslint-plugin-markdown was installed into a node_modules folder on the same level as eslint. However, that doesn't work here since eslint is in tools/, so the plugin is installed into tools/eslint/node_modules/ instead. This works, but it doesn't seem ideal (e.g. if ESLint is reinstalled, its node_modules folder might be purged so we might forget to reinstall eslint-plugin-markdown). Where would be the best place to put the plugin?

not-an-aardvark · 2017-04-21T20:17:38Z

@vsemozhetbyt

To add this to the build process, I think you would need to replace these lines with:

$(NODE) tools/eslint/bin/eslint.js --cache --rulesdir=tools/eslint-rules --ext=.js,.md \
  benchmark lib test tools docs

In other words, add docs to the list of globs to be linted, and also add .md to the list of default file extensions to lint. (On its own this would override the default .js extension, so also add .js to make sure JS files are linted as well.)

To ignore files like doc/api_assets, you can add globs to the .eslintignore file.

Dismissing my review for now because a few new commits have been added.

vsemozhetbyt · 2017-04-21T20:24:13Z

~~Should this glob be doc/api_assets/*.js or a wider glob to be safe? I am not very good at globs here. How can we exclude any *.js files in the doc and all subdirectories with any depth?~~

Hopefully, make all *.js files in doc ignored.

Trott · 2017-04-21T20:28:46Z

I'm not sure about the best place to put eslint-plugin-markdown.

@not-an-aardvark I'm not sure either. If we were starting from scratch, I'd probably say put everything (including eslint itself) in tools/eslint/node_modules and maybe put some symlinks in tools/eslint to tools/eslint/node_modules/eslint.

As is, I'm OK with putting eslint-plugin-markdown into tools/eslint/node_modules as long as things break in an understandable way when the plugin is missing after an upgrade. For that matter, the upgrade process probably needs to be documented somewhere and that would be a good place to include the instructions to include the plugin. I might try to do that if I get some time today, but as always, I'd be thrilled if someone beats me to it. (I always forget a step in the upgrade anyway involving a symlink or something to .bin/eslint.js I think? @sam-github is usually the person that fixes it when I make that error, which I'm pretty sure I've made more than once. Will have to go through git history and figure it out unless my @-mention above summons Sam and Sam's memory is solid on it.)

TimothyGu

Nice work!

TimothyGu · 2017-04-21T20:28:08Z

doc/api/cluster.md

I'd prefer the older more concise style, if possible.

TimothyGu · 2017-04-21T20:29:23Z

doc/api/console.md

Would

for (let i = 0; i < 100; i++) {}

work?

Some people may be unfamiliar with the syntax of an empty statement.

TimothyGu · 2017-04-21T20:30:11Z

doc/api/crypto.md

4-space indent, and break the string into multiple lines using + if needed.

TimothyGu · 2017-04-21T20:30:58Z

doc/api/fs.md

You can just remove the language.

TimothyGu · 2017-04-21T20:32:22Z

doc/api/modules.md

I feel in this case the code block should just be inlined, as

... directly by testing require.main === module.

TimothyGu · 2017-04-21T20:34:05Z

doc/api/console.md

This will result in myConsole.assert.name === 'value', which may not be desirable. I'd add an exclusion for func-name-matching.

TimothyGu · 2017-04-21T20:35:29Z

doc/api/process.md

Ditto about txt language. Or, disabling ESLint for this block and keep js also works for me.

TimothyGu · 2017-04-21T20:36:54Z

doc/api/readline.md

Can this instead be:

const hits = completions.filter((c) => c.indexOf(line) === 0);

TimothyGu · 2017-04-21T20:38:45Z

doc/api/zlib.md

The semicolon looks out of place here as well.

TimothyGu · 2017-04-21T20:39:07Z

doc/api/zlib.md

Ditto. This should be short enough to make it inlined.

vsemozhetbyt · 2017-04-21T20:46:57Z

~~Some more questions:~~

~~Should plugins: [markdown] be placed in doc/.eslintrc.yaml or root .eslintrc.yaml ?~~ Placed in root .eslintrc.yaml to check docs in other folders.
~~Should we also add to .eslintignore all .md files in non-doc folders?~~ Currently no need.

hiroppy · 2017-04-21T20:49:43Z

doc/api/stream.md

You can use let.

vsemozhetbyt · 2017-04-21T21:34:24Z

@TimothyGu @abouthiroppy Thank you. Comments hopefully addressed.

TimothyGu

LGTM after one last nit.

TimothyGu · 2017-04-21T21:58:09Z

doc/api/zlib.md

This empty line can now be removed.

vsemozhetbyt · 2017-04-21T22:19:06Z

@not-an-aardvark

*.js files in doc folder added to .eslintignore
Currently, this command find no problems:

eslint --rulesdir tools/eslint-rules/ --ext=.js,.md benchmark doc lib test tools

So maybe no need to filter any more files: all .md in benchmark lib test tools are linted successfully (I've added some directives in the test/README.md and tools/icu/README.md),

Trying to settle build configs...

hiroppy · 2017-04-21T22:38:51Z

LGTM. Great work! Thank you, @vsemozhetbyt 🍻

vsemozhetbyt · 2017-04-21T22:45:58Z

Makefile and vcbuild.bat hopefully updated.

vsemozhetbyt · 2017-04-21T22:49:55Z

Let's start with Linter CI: https://ci.nodejs.org/job/node-test-linter/8451/

PR-URL: #12640 Fixes: #12635 Refs: #12563 Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

gibfahn · 2017-05-16T14:50:43Z

I think this should bake for another release first, but after the next v6.x release comes out @vsemozhetbyt would you be willing to backport to v6.x? Guide is here. If it shouldn't be landed let me know and we can add the dont-land label.

vsemozhetbyt · 2017-05-16T14:57:28Z

@gibfahn I can try. Just let me know when this can be done.
BTW, the #12640 is the necessary completion for this PR.

vsemozhetbyt · 2017-05-16T14:58:45Z

In addition, I am not sure if we should wait for eslint/markdown#69 fixed.

gibfahn · 2017-05-16T16:13:52Z

@vsemozhetbyt sounds good, I'll ask again in a month!

gibfahn · 2017-06-18T19:15:04Z

Looks like eslint/markdown#69 is really close to landing. Once it does and you're comfortable with it (I guess we need to pull in the fix too) feel free to raise the backport PR.

vsemozhetbyt · 2017-07-03T16:48:00Z

@gibfahn Should I wait 2 weeks after #14047 landing to raise the backport PR combined from all three: #12563 + #12640 + #14047 ?

gibfahn · 2017-07-03T17:24:01Z

@gibfahn Should I wait 2 weeks after #14047 landing to raise the backport PR combined from all three: #12563 + #12640 + #14047 ?

The 2 week wait doesn't normally apply to build tools, and I don't think updating the dependency is a particularly dangerous thing to do, so I'd say raise it now if you'd like.

vsemozhetbyt · 2017-07-04T00:47:23Z

@gibfahn Well, that was something. PTAL: #14067

See also the first comment. Sorry for the mess, I do not know how to do this easier(

This is an initial step to eliminate most of parsing errors. Backport-PR-URL: #14067 PR-URL: #12563 Refs: #12557 (comment) Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>

Backport-PR-URL: #14067 PR-URL: #12563 Refs: #12557 (comment) Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>

* Install eslint-plugin-markdown@1.0.0-beta.7 * Add doc/.eslintrc.yaml * Add `plugins: [markdown]` to the main .eslintrc.yaml * .js files in doc folder added to .eslintignore * Update Makefile, vcbuild.bat, and tools/jslint.js Refs: #12563 Refs: #12640 Refs: #14047 PR-URL: #14067 Reviewed-By: James Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

This is an initial step to eliminate most of parsing errors. Backport-PR-URL: #14067 PR-URL: #12563 Refs: #12557 (comment) Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>

Backport-PR-URL: #14067 PR-URL: #12563 Refs: #12557 (comment) Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>

* Install eslint-plugin-markdown@1.0.0-beta.7 * Add doc/.eslintrc.yaml * Add `plugins: [markdown]` to the main .eslintrc.yaml * .js files in doc folder added to .eslintignore * Update Makefile, vcbuild.bat, and tools/jslint.js Refs: #12563 Refs: #12640 Refs: #14047 PR-URL: #14067 Reviewed-By: James Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

This is an initial step to eliminate most of parsing errors. Backport-PR-URL: #14067 PR-URL: #12563 Refs: #12557 (comment) Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>

Backport-PR-URL: #14067 PR-URL: #12563 Refs: #12557 (comment) Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>

* Install eslint-plugin-markdown@1.0.0-beta.7 * Add doc/.eslintrc.yaml * Add `plugins: [markdown]` to the main .eslintrc.yaml * .js files in doc folder added to .eslintignore * Update Makefile, vcbuild.bat, and tools/jslint.js Refs: #12563 Refs: #12640 Refs: #14047 PR-URL: #14067 Reviewed-By: James Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

vsemozhetbyt added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Apr 21, 2017

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Apr 21, 2017

not-an-aardvark previously approved these changes Apr 21, 2017

View reviewed changes

thefourtheye reviewed Apr 21, 2017

View reviewed changes

vsemozhetbyt added the wip Issues and PRs that are still a work in progress. label Apr 21, 2017

TimothyGu reviewed Apr 21, 2017

View reviewed changes

hiroppy reviewed Apr 21, 2017

View reviewed changes

doc/api/stream.md Outdated

Copy link

Member

hiroppy Apr 21, 2017

Choose a reason for hiding this comment

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

You can use let.

vsemozhetbyt reacted with thumbs up emoji

vsemozhetbyt mentioned this pull request Apr 21, 2017

doc: unify trailing commas in code examples #12557

Closed

2 tasks

TimothyGu reviewed Apr 21, 2017

View reviewed changes

not-an-aardvark approved these changes Apr 21, 2017

View reviewed changes

vsemozhetbyt mentioned this pull request May 12, 2017

 does not work in the v1.0.0-beta.6 eslint/markdown#69

Closed

gibfahn added baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels May 16, 2017

jasnell mentioned this pull request May 11, 2017

8.0.0 Release Proposal #12220

Closed

vsemozhetbyt mentioned this pull request Jul 4, 2017

[v6.x backport] build, doc, tools: use eslint-plugin-markdown #14067

Closed

3 tasks

refack mentioned this pull request Jul 4, 2017

tools: add quote-props eslint rule #14059

Closed

4 tasks

MylesBorins added backported-to-v6.x and removed backport-requested-v6.x baking-for-lts PRs that need to wait before landing in a LTS release. labels Jul 21, 2017

MylesBorins mentioned this pull request Jul 21, 2017

v6.11.2 proposal #14356

Merged

Uh oh!

Conversation

vsemozhetbyt commented Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Commit 1: an initial step to eliminate parsing errors. Fixing strategies:

Commit 2: conform most of the problematic code to the rules.

Commit 3: add eslint-plugin-markdown stuff

Commit 4: make doc linting a bit more specific (as we do for tests code)

Uh oh!

not-an-aardvark commented Apr 21, 2017

Uh oh!

vsemozhetbyt commented Apr 21, 2017

Uh oh!

vsemozhetbyt commented Apr 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt commented Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vsemozhetbyt commented Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

not-an-aardvark commented Apr 21, 2017

Uh oh!

vsemozhetbyt commented Apr 21, 2017

Uh oh!

vsemozhetbyt commented Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

not-an-aardvark commented Apr 21, 2017

Uh oh!

not-an-aardvark commented Apr 21, 2017

Uh oh!

vsemozhetbyt commented Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Apr 21, 2017

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimothyGu Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt commented Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

vsemozhetbyt commented Apr 21, 2017 •

edited

Loading

vsemozhetbyt Apr 21, 2017 •

edited

Loading

vsemozhetbyt commented Apr 21, 2017 •

edited

Loading

vsemozhetbyt commented Apr 21, 2017 •

edited

Loading

vsemozhetbyt commented Apr 21, 2017 •

edited

Loading

vsemozhetbyt commented Apr 21, 2017 •

edited

Loading

TimothyGu Apr 21, 2017 •

edited

Loading

vsemozhetbyt commented Apr 21, 2017 •

edited

Loading

vsemozhetbyt commented Apr 21, 2017 •

edited

Loading

hiroppy commented Apr 21, 2017 •

edited

Loading

gibfahn commented Jun 18, 2017 •

edited

Loading

vsemozhetbyt commented Jul 3, 2017 •

edited

Loading

vsemozhetbyt commented Jul 4, 2017 •

edited

Loading