doc: use eslint #5053

tflanagan · 2016-02-03T06:34:54Z

This PR enforces eslint on JavaScript code blocks in the documentation.

When the docs are generated as HTML, the inline eslint rules are removed from the output.

This PR also introduces the make jslint-docs command and the eslint-plugin-markdown module in tools/

i suppose vcbuild.bat could have jslint-docs pulled out, like it is in Makefile

chrisdickinson · 2016-02-03T07:07:03Z

Excellent work!

The Docs WG is looking at doing this through remark ultimately, but that shouldn't block this since it gets the docs in a good state for the other tool.

tflanagan · 2016-02-03T18:36:21Z

@chrisdickinson, thanks. Started out with over 1500 linting errors...

I've just added buffer.markdown, making this PR complete, minus reviews

thefourtheye · 2016-02-03T19:11:32Z

I pulled this PR locally with

git fetch iojs pull/5053/head:pr-5053
git checkout pr-5053

as shown in https://help.github.com/articles/checking-out-pull-requests-locally/.

And then I listed out the changed files with

git --no-pager diff --name-only FETCH_HEAD $(git merge-base FETCH_HEAD master)

as shown in http://stackoverflow.com/a/25071749/1903116.

And then I filtered the files which had node_modules in them with

git --no-pager diff --name-only FETCH_HEAD $(git merge-base FETCH_HEAD master) | grep -v node_modules

which gave me

As decided in the docs WG meeting today, if we can get LGTM atleast to the modules listed above, we are okay to land this. cc @nodejs/documentation Please correct me if I am wrong

tflanagan · 2016-02-03T19:56:07Z

@thefourtheye that list looks correct.

I wasn't sure what to do with the eslint-plugin-markdown code (the node_modules), so I shoved it in tools.

chrisdickinson · 2016-02-03T19:59:08Z

Sending this comment while on the go, forgive me if this is where they already are — There should be a tools/doc/node_modules dir.

tflanagan · 2016-02-03T20:05:52Z

@chrisdickinson not sure how you would solve the eslint package finding the eslint-plugin-markdown package. The reason this PR works is because node_modules is in the hierarchy of where eslint lives.

Trott · 2016-02-03T22:29:19Z

I would favor tools/eslint/node_modules over tools/node_modules as the location to put an eslint plugin. Nothing else needs access to it. As much as possible, keeping all the eslint stuff in one place strikes me as a Good Idea.

Trott · 2016-02-03T22:30:42Z

Of course, the downside there is that someone will probably have to reinstall the plugin there every time we upgrade eslint as our process is likely to completely replace the tools/eslint directory. So /shrug....

silverwind · 2016-02-04T18:40:00Z

If it's possible, I'd prefer plugins to live in tools/eslint-plugins.

tflanagan · 2016-02-05T03:42:09Z

@silverwind, @chrisdickinson, @Trott How does this look?

It essentially just renames node_modules to eslint-plugins, it has the indirect truth of only containing modules that eslint uses i guess.

Wasn't sure how else to accomplish this. Little ugly, but gets it done

thefourtheye · 2016-02-05T04:35:26Z

I pulled in the latest changes, and I got the following error.

➜  io.js git:(pr-5053) make lint
./node tools/eslint/bin/eslint.js lib src test tools/doc tools/eslint-rules \
        --rulesdir tools/eslint-rules --quiet
export NODE_PATH="tools/eslint-plugins"
./node tools/eslint/bin/eslint.js --plugin eslint-plugin-markdown --ext markdown doc/api/ \
        --rulesdir tools/eslint-rules --rule "strict:0" --rule "eol-last:0" --quiet
module.js:341
    throw err;
    ^

Error: Cannot find module 'eslint-plugin-markdown'
    at Function.Module._resolveFilename (module.js:339:15)
    at Function.Module._load (module.js:290:25)
    at Module.require (module.js:367:17)
    at require (internal/module.js:16:19)
    at /home/thefourtheye/git/io.js/tools/eslint/lib/cli-engine.js:125:26
    at Array.forEach (native)
    at loadPlugins (/home/thefourtheye/git/io.js/tools/eslint/lib/cli-engine.js:116:21)
    at processText (/home/thefourtheye/git/io.js/tools/eslint/lib/cli-engine.js:204:5)
    at processFile (/home/thefourtheye/git/io.js/tools/eslint/lib/cli-engine.js:270:18)
    at executeOnFile (/home/thefourtheye/git/io.js/tools/eslint/lib/cli-engine.js:617:23)
make: *** [jslint-docs] Error 1

Trott · 2016-02-05T05:10:10Z

@tflanagan Works for me if it works and no one else objects. We can always switch it to some other layout later if a more elegant solution presents itself.

tflanagan · 2016-02-05T13:24:43Z

@thefourtheye, I'm sorry about that - I was developing (and testing) on Windows when I made this change, my linux box was still compiling when I dozed off for the night

Should be all set now.

evanlucas · 2016-02-05T13:29:39Z

-1 on using NODE_PATH. That is something we are trying to get rid of. If people see us use it, they will think it is ok for them to use. What's the problem with just using node_modules?

a0viedo · 2016-02-05T14:40:45Z

doc/api/child_process.markdown

@@ -160,7 +162,7 @@ to be an error.
 The `options` argument may be passed as the second argument to customize how
 the process is spawned. The default options are:

-```js
+```json


Be aware that changing the markdown from js to json requires a valid JSON object, with all the keys and string values having double quotes.

@a0viedo, ah, thank you! I will fix this shortly.

@a0viedo, I've addressed all the "```json"s

tflanagan · 2016-02-05T16:46:27Z

@evanlucas I see no problem with using it, I foresee other tools requiring this need as well, but I also understand why eslint-plugins might be nice to separate things out

tflanagan · 2016-02-09T23:50:53Z

As I can see other tools requiring a node_modules folder, and the -1/near deprecation on using NODE_PATH, I am going to rebase and resolve the conflicts, and revert the change from node_modules to eslint-plugins.

Hoping to get some LGTM's and get this landed soon after to avoid further conflicts

Adds the esling-plugin-markdown module and its' dependencies to the tools/node_modules/ directory for use by tools/eslint.

Add the "jslint-docs" command to Makefile and vcbuild.bat. This command adds linting of the documentation javascript code blocks

Fix code examples to pass eslint

Fix vm.markdown so that it passes eslint

tflanagan · 2016-02-12T16:53:37Z

To further explain my reasoning, it much easier to detect and knowingly, manually declare something as no-undef than it is to start at default as no-undef and then go and find those errors manually.

tflanagan · 2016-02-12T16:55:19Z

Here is a breakdown of how many instances of inline linter comments:

doc/api/ > grep -c "/* eslint" *
_toc.markdown:0
addons.markdown:2
all.markdown:0
assert.markdown:0
buffer.markdown:3
child_process.markdown:2
cluster.markdown:2
console.markdown:4
crypto.markdown:8
debugger.markdown:2
dgram.markdown:2
dns.markdown:0
documentation.markdown:0
domain.markdown:4
errors.markdown:3
events.markdown:5
fs.markdown:3
globals.markdown:0
http.markdown:12
https.markdown:2
index.markdown:0
modules.markdown:2
net.markdown:4
os.markdown:0
path.markdown:0
process.markdown:6
punycode.markdown:0
querystring.markdown:2
readline.markdown:3
repl.markdown:1
stream.markdown:26
string_decoder.markdown:0
synopsis.markdown:0
timers.markdown:0
tls.markdown:2
tty.markdown:0
url.markdown:0
util.markdown:3
v8.markdown:0
vm.markdown:2
zlib.markdown:0

tflanagan · 2016-02-17T15:15:07Z

Any thoughts? Trying not to stall out

jasnell · 2016-02-17T16:46:56Z

@nodejs/ctc @nodejs/documentation ... ping

eljefedelrodeodeljefe · 2016-02-17T16:53:18Z

@thefourtheye had some stake, I believe.

Knighton910 · 2016-02-17T18:47:24Z

part of nodejs/docs team pinging in here. There were a few things i liked before, but overall i feel your
pull req will give us more cleaner code. #LGTM

silverwind · 2016-02-17T22:31:18Z

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

silverwind · 2016-02-17T22:48:23Z

Looks like we need another rebase against master.

tflanagan · 2016-02-17T23:47:18Z

@silverwind Im busy now, but I'll have it rebased tomorrow, thanks for your help

jasnell · 2016-04-21T17:49:42Z

ping... @tflanagan .. just checking in with you on this. Any updates?

silverwind · 2016-04-21T17:53:03Z

Note that .markdown -> .md happened.

yorkie · 2016-05-03T03:25:32Z

Any progress on this PR, guys?

Trott · 2016-09-16T23:28:41Z

@tflanagan Any chance of calling you out of Node.js core retirement for a rebase on this? I'd kinda like to see something like this land, and I can try to push it forward if you are short on availability and/or interest right now.

(I'd also like to see you come out of Node.js core retirement for a commit or two so we can get you onboarded as a collaborator, because we totally should have done that when you were more active. Apologies for the oversight.)

eljefedelrodeodeljefe · 2016-09-17T09:22:13Z

I am gonna pick this up today and tomorrow during Collaborator Summit. Seems a good opportunity too get more active too and also I am responsible for the breakages. I'll cherry pick the structual changes, since everything else has move to much anyhow.

eljefedelrodeodeljefe · 2016-09-17T10:08:03Z

Reviewed. It's not possible to go forward with since it requires remark. Gonna continue working on that issue. Was mentioned several times before.

tflanagan changed the title ~~Docs use eslint~~ doc: use eslint Feb 3, 2016

Trott added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Feb 3, 2016

tflanagan force-pushed the docs-use-eslint branch 2 times, most recently from 081771e to 7f2baf4 Compare February 3, 2016 18:30

jasnell added the lts-watch-v4.x label Feb 4, 2016

tflanagan force-pushed the docs-use-eslint branch from 3a908bb to d1e07be Compare February 5, 2016 03:40

a0viedo reviewed Feb 5, 2016
View reviewed changes

tflanagan added 2 commits February 10, 2016 01:54

tools: add eslint-plugin-markdown to tools

06b7a92

Adds the esling-plugin-markdown module and its' dependencies to the tools/node_modules/ directory for use by tools/eslint.

build: add jslint-docs

7ef37e7

Add the "jslint-docs" command to Makefile and vcbuild.bat. This command adds linting of the documentation javascript code blocks

tflanagan force-pushed the docs-use-eslint branch 2 times, most recently from 9a77e41 to 3f7de23 Compare February 10, 2016 08:49

tflanagan added 2 commits February 10, 2016 10:53

doc: fix zlib.markdown code examples

b1d72c4

Fix code examples to pass eslint

doc: fix vm.markdown code examples

f33102f

Fix vm.markdown so that it passes eslint

tflanagan force-pushed the docs-use-eslint branch 2 times, most recently from 5fbc910 to e14f00f Compare February 17, 2016 20:18

build,doc: moving doc lint rules to doc/api/.eslintrc

5d58b3f

tflanagan force-pushed the docs-use-eslint branch from e14f00f to 5d58b3f Compare February 17, 2016 20:23

build: add jslint-docs to test

cb5c1dd

targos mentioned this pull request Feb 22, 2016

tools: apply custom buffer lint rule to /lib only #5371

Closed

bengl mentioned this pull request Apr 20, 2016

doc: fix style inconsistencies in code examples #6313

Closed

2 tasks

estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:23

jasnell added the stalled Issues and PRs that are stalled. label Apr 28, 2016

silverwind mentioned this pull request May 2, 2016

doc,cluster: fix the lint of an example #6516

Closed

4 tasks

janriemer mentioned this pull request May 5, 2016

doc: missing require calls in some code examples #6596

Closed

eljefedelrodeodeljefe closed this Sep 17, 2016

MylesBorins removed the lts-watch-v4.x label Oct 10, 2016

mbrevda mentioned this pull request Dec 1, 2023

Add --env-file-if-exists #50993

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: use eslint #5053

doc: use eslint #5053

tflanagan commented Feb 3, 2016

chrisdickinson commented Feb 3, 2016

tflanagan commented Feb 3, 2016

thefourtheye commented Feb 3, 2016

tflanagan commented Feb 3, 2016

chrisdickinson commented Feb 3, 2016

tflanagan commented Feb 3, 2016

Trott commented Feb 3, 2016

Trott commented Feb 3, 2016

silverwind commented Feb 4, 2016

tflanagan commented Feb 5, 2016

thefourtheye commented Feb 5, 2016

Trott commented Feb 5, 2016

tflanagan commented Feb 5, 2016

evanlucas commented Feb 5, 2016

a0viedo Feb 5, 2016

tflanagan Feb 5, 2016

tflanagan Feb 5, 2016

tflanagan commented Feb 5, 2016

tflanagan commented Feb 9, 2016

tflanagan commented Feb 12, 2016

tflanagan commented Feb 12, 2016

tflanagan commented Feb 17, 2016

jasnell commented Feb 17, 2016

eljefedelrodeodeljefe commented Feb 17, 2016

Knighton910 commented Feb 17, 2016

silverwind commented Feb 17, 2016

silverwind commented Feb 17, 2016

tflanagan commented Feb 17, 2016

jasnell commented Apr 21, 2016

silverwind commented Apr 21, 2016

yorkie commented May 3, 2016

Trott commented Sep 16, 2016

eljefedelrodeodeljefe commented Sep 17, 2016

eljefedelrodeodeljefe commented Sep 17, 2016

doc: use eslint #5053

doc: use eslint #5053

Conversation

tflanagan commented Feb 3, 2016

chrisdickinson commented Feb 3, 2016

tflanagan commented Feb 3, 2016

thefourtheye commented Feb 3, 2016

tflanagan commented Feb 3, 2016

chrisdickinson commented Feb 3, 2016

tflanagan commented Feb 3, 2016

Trott commented Feb 3, 2016

Trott commented Feb 3, 2016

silverwind commented Feb 4, 2016

tflanagan commented Feb 5, 2016

thefourtheye commented Feb 5, 2016

Trott commented Feb 5, 2016

tflanagan commented Feb 5, 2016

evanlucas commented Feb 5, 2016

a0viedo Feb 5, 2016

Choose a reason for hiding this comment

tflanagan Feb 5, 2016

Choose a reason for hiding this comment

tflanagan Feb 5, 2016

Choose a reason for hiding this comment

tflanagan commented Feb 5, 2016

tflanagan commented Feb 9, 2016

tflanagan commented Feb 12, 2016

tflanagan commented Feb 12, 2016

tflanagan commented Feb 17, 2016

jasnell commented Feb 17, 2016

eljefedelrodeodeljefe commented Feb 17, 2016

Knighton910 commented Feb 17, 2016

silverwind commented Feb 17, 2016

silverwind commented Feb 17, 2016

tflanagan commented Feb 17, 2016

jasnell commented Apr 21, 2016

silverwind commented Apr 21, 2016

yorkie commented May 3, 2016

Trott commented Sep 16, 2016

eljefedelrodeodeljefe commented Sep 17, 2016

eljefedelrodeodeljefe commented Sep 17, 2016