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

doc, tools: use eslint-plugin-markdown #12563

Closed
wants to merge 4 commits into from
Closed

doc, tools: use eslint-plugin-markdown #12563

wants to merge 4 commits into from

Conversation

vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Apr 21, 2017

Checklist
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:
  • Terminal / REPL interaction logs in repl.md: add <!-- eslint-disable --> 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 <!-- eslint-disable rule --> or <!-- eslint-disable --> 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.

@vsemozhetbyt 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 nodejs-github-bot added the doc Issues and PRs related to the documentations. label Apr 21, 2017
@not-an-aardvark
Copy link
Contributor

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
Copy link
Contributor Author

@not-an-aardvark I've reverted <!-- eslint-disable --> part for repl.md for now.

@vsemozhetbyt
Copy link
Contributor Author

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

doc/api/dns.md Outdated
@@ -353,15 +353,15 @@ be an object with the following properties:
* `minttl`

```js
{
({
Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@vsemozhetbyt vsemozhetbyt Apr 21, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 vsemozhetbyt added the wip Issues and PRs that are still a work in progress. label Apr 21, 2017
@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Apr 21, 2017

@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
Copy link
Contributor Author

vsemozhetbyt commented Apr 21, 2017

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

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

@not-an-aardvark
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

vsemozhetbyt commented Apr 21, 2017

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

@not-an-aardvark
Copy link
Contributor

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
Copy link
Contributor

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

@not-an-aardvark not-an-aardvark dismissed their stale review April 21, 2017 20:18

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

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Apr 21, 2017

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
Copy link
Member

Trott commented Apr 21, 2017

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

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

Nice work!

worker.process.pid, signal || code);
cluster.fork();
}
);
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

Would

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

work?

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

@@ -288,7 +288,8 @@ decipher.on('end', () => {
// Prints: some clear text data
});

const encrypted = 'ca981be48e90867604588e75d04feabb63cc007a8f8ad89b10616ed84d815504';
const encrypted =
'ca981be48e90867604588e75d04feabb63cc007a8f8ad89b10616ed84d815504';
Copy link
Member

Choose a reason for hiding this comment

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

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

doc/api/fs.md Outdated
@@ -217,7 +217,7 @@ synchronous counterparts are of this type.
For a regular file [`util.inspect(stats)`][] would return a string very
similar to this:

```js
```txt
Copy link
Member

Choose a reason for hiding this comment

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

You can just remove the language.

@@ -70,7 +70,7 @@ When a file is run directly from Node.js, `require.main` is set to its
directly by testing

```js
require.main === module
require.main === module;
Copy link
Member

Choose a reason for hiding this comment

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

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

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

@@ -139,7 +142,7 @@ the default behavior of `console` in Node.js.
// new impl for assert without monkey-patching.
const myConsole = Object.create(console, {
assert: {
value: function assert(assertion, message, ...args) {
value(assertion, message, ...args) {
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -529,7 +528,7 @@ running the `./configure` script.

An example of the possible output looks like:

```js
```txt
Copy link
Member

@TimothyGu TimothyGu Apr 21, 2017

Choose a reason for hiding this comment

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

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

@@ -414,7 +414,7 @@ For instance: `[[substr1, substr2, ...], originalsubstring]`.
```js
function completer(line) {
const completions = '.help .error .exit .quit .q'.split(' ');
const hits = completions.filter((c) => { return c.indexOf(line) === 0 });
const hits = completions.filter((c) => { return c.indexOf(line) === 0; });
Copy link
Member

Choose a reason for hiding this comment

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

Can this instead be:

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

doc/api/zlib.md Outdated
@@ -149,7 +152,7 @@ From `zlib/zconf.h`, modified to node.js's usage:
The memory requirements for deflate are (in bytes):

```js
(1 << (windowBits+2)) + (1 << (memLevel+9))
(1 << (windowBits + 2)) + (1 << (memLevel + 9));
Copy link
Member

Choose a reason for hiding this comment

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

The semicolon looks out of place here as well.

doc/api/zlib.md Outdated
```

This will, however, generally degrade compression.

The memory requirements for inflate are (in bytes)

```js
1 << windowBits
1 << windowBits;
Copy link
Member

Choose a reason for hiding this comment

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

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

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Apr 21, 2017

Some more questions:

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

@@ -280,7 +280,7 @@ has been called, and all data has been flushed to the underlying system.

```js
const writer = getWritableStreamSomehow();
for (var i = 0; i < 100; i ++) {
for (var i = 0; i < 100; i++) {
Copy link
Member

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
Copy link
Contributor Author

@TimothyGu @abouthiroppy Thank you. Comments hopefully addressed.

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

LGTM after one last nit.

doc/api/zlib.md Outdated
```js
1 << windowBits
```
The memory requirements for inflate are (in bytes) `1 << windowBits`.

Copy link
Member

Choose a reason for hiding this comment

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

This empty line can now be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Apr 21, 2017

@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
Copy link
Member

hiroppy commented Apr 21, 2017

LGTM. Great work! Thank you, @vsemozhetbyt 🍻

@vsemozhetbyt
Copy link
Contributor Author

Makefile and vcbuild.bat hopefully updated.

@vsemozhetbyt
Copy link
Contributor Author

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

evanlucas pushed a commit that referenced this pull request May 2, 2017
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 gibfahn added baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels May 16, 2017
@gibfahn
Copy link
Member

gibfahn commented May 16, 2017

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
Copy link
Contributor Author

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

@vsemozhetbyt
Copy link
Contributor Author

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

@gibfahn
Copy link
Member

gibfahn commented May 16, 2017

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

@jasnell jasnell mentioned this pull request May 11, 2017
@gibfahn
Copy link
Member

gibfahn commented Jun 18, 2017

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
Copy link
Contributor Author

vsemozhetbyt commented Jul 3, 2017

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

@gibfahn
Copy link
Member

gibfahn commented Jul 3, 2017

@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
Copy link
Contributor Author

vsemozhetbyt commented Jul 4, 2017

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

@refack refack mentioned this pull request Jul 4, 2017
4 tasks
MylesBorins pushed a commit that referenced this pull request Jul 21, 2017
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>
MylesBorins pushed a commit that referenced this pull request Jul 21, 2017
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>
MylesBorins pushed a commit that referenced this pull request Jul 21, 2017
* 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>
@MylesBorins 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 pushed a commit that referenced this pull request Jul 21, 2017
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>
MylesBorins pushed a commit that referenced this pull request Jul 21, 2017
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>
MylesBorins pushed a commit that referenced this pull request Jul 21, 2017
* 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>
@MylesBorins MylesBorins mentioned this pull request Jul 21, 2017
MylesBorins pushed a commit that referenced this pull request Jul 31, 2017
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>
MylesBorins pushed a commit that referenced this pull request Jul 31, 2017
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>
MylesBorins pushed a commit that referenced this pull request Jul 31, 2017
* 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>
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. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.