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

introduce doc linter #11610

Closed
wants to merge 34 commits into from
Closed

Conversation

thefourtheye
Copy link
Contributor

This is an attempt to introduce a simple linter for our documentation files.

The doc linter scans all the .md files in the doc/api directory and
invokes the linter rule functions one by one, passing the file name,
contents and an array to fill the errors.

If the errors array is not empty, then the linter will exit with code 1

This PR has one rule, which checks if the links in the documentation file are in sorted order. It expects alphabetical link text to be at the top and everything else following them.


Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

build doc tools


cc @nodejs/documentation

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Feb 28, 2017
@addaleax
Copy link
Member

Also /cc @ChALkeR, I think he was working on something based on remark-lint? We do have a .remarkrc in our top level directory

@ashleygwilliams
Copy link
Contributor

i know +1 comments are kind of a pain, but i just want to say that this is super cool and i'd love to use this in a lot of other projects as well. thanks for all this work @thefourtheye !

@mscdex
Copy link
Contributor

mscdex commented Feb 28, 2017

A couple of immediate observations:

  • I think the ordering should not be case sensitive and should disregard special characters at the beginning (e.g. backticks).

  • Awhile back when I was working on doc improvements, I had started to move external links to a separate sorted list for better organization (IMO). So at the very least I personally would like to see the changes where these lists are recombined, reverted. Ideally we would have these separate lists for every doc file, I just never got around to doing it everywhere (mostly because it was part of a larger set of changes for each doc file).

  • It would be nice to be able to lint for other things like missing argument and property types and maybe event verify the types themselves.

@sam-github
Copy link
Contributor

I am a big fan of sorting.

I don't think we should invent sort orders (particularly ones that skip punctuation), lets just use the standard one, aka the "C" Locale, aka the default in Unix, aka ASCII sort order. This allows most programmer's editors to block select and sort out-of-the-box with no special configuration, or to just block select and pipe through the sort utility.

@Qard
Copy link
Member

Qard commented Feb 28, 2017

@addaleax There was indeed some work from the @nodejs/documentation wg to use remark-lint, but that was originally happening as part of a full conversion of the doc tool from marked to remark which stalled, sadly.

@Trott
Copy link
Member

Trott commented Mar 1, 2017

Here's @ChALkeR's PR (still open!) to do something similar but to use remark as the linting tool: #8551

@eljefedelrodeodeljefe
Copy link
Contributor

I am rather happy, that we can do it w/o external deps. Generally in favour.

@thefourtheye
Copy link
Contributor Author

@mscdex

I think the ordering should not be case sensitive and should disregard special characters at the beginning (e.g. backticks).

In that case, the sorted could look like this,

[Abcd]: ...
[`efgh`]: ...
[IJKL]: ...

That doesn't look pleasant to my eyes, at least.

move external links to a separate sorted list for better organization

I am sure that would look good, but we would lose the ability use editors' autosort functionality, as pointed out by @sam-github.

It would be nice to be able to lint for other things like missing argument and property types and maybe event verify the types themselves.

Well, the linter can lint those by adding more modules/functions.

@ChALkeR
Copy link
Member

ChALkeR commented Mar 7, 2017

@thefourtheye, I had an alternate PR in #8551, that wasn't finished because I didn't make it produce tap tests results for the CI.

#8551 got closed and I didn't notice that before updating the branch just now, so I can't reopen it, but if you take a look at the https://github.com/ChALkeR/io.js/tree/remark branch, you will see that it's hinting some issues atm:

$ make doclint
./node tools/remark-cli/cli.js -qf *.md doc src lib benchmark tools/doc/*.md tools/icu/*.md
doc/api/assert.md
              1:1  warning  Missing newline character at end of file       final-newline                  remark-lint

doc/api/child_process.md
   1219:1-1219:48  warning  Found unused definition                        no-unused-definitions          remark-lint
  1250:1-1250:108  warning  Found unused definition                        no-unused-definitions          remark-lint

doc/api/crypto.md
  2038:1-2038:105  warning  Found unused definition                        no-unused-definitions          remark-lint
  2040:1-2040:115  warning  Found unused definition                        no-unused-definitions          remark-lint

doc/api/debugger.md
            175:1  warning  Missing caret in blockquote                    no-blockquote-without-caret    remark-lint

doc/api/dgram.md
     497:1-497:24  warning  Found unused definition                        no-unused-definitions          remark-lint

doc/api/fs.md
  2509:1-2509:108  warning  Found unused definition                        no-unused-definitions          remark-lint

doc/api/http.md
   1743:1-1743:47  warning  Found unused definition                        no-unused-definitions          remark-lint

doc/api/net.md
     987:1-987:38  warning  Found unused definition                        no-unused-definitions          remark-lint

doc/guides/maintaining-V8.md
         3:1-3:13  warning  Don’t use multiple top level headings (3:1)    no-multiple-toplevel-headings  remark-lint
       16:1-16:22  warning  Don’t use multiple top level headings (16:1)   no-multiple-toplevel-headings  remark-lint
       40:1-40:31  warning  Don’t use multiple top level headings (40:1)   no-multiple-toplevel-headings  remark-lint
     124:1-124:22  warning  Don’t use multiple top level headings (124:1)  no-multiple-toplevel-headings  remark-lint
      182:1-200:4  warning  Missing code-language flag                     fenced-code-flag               remark-lint
     223:1-223:14  warning  Don’t use multiple top level headings (223:1)  no-multiple-toplevel-headings  remark-lint
     288:1-288:51  warning  Don’t use multiple top level headings (288:1)  no-multiple-toplevel-headings  remark-lint
     307:1-307:68  warning  Don’t use multiple top level headings (307:1)  no-multiple-toplevel-headings  remark-lint

doc/changelogs/CHANGELOG_V6.md
   962:95-962:107  warning  Don’t pad `emphasis` with inner spaces         no-inline-padding              remark-lint

doc/releases.md
      298:1-302:4  warning  Missing code-language flag                     fenced-code-flag               remark-lint

⚠ 20 warnings

Returning to this PR, do we want to write all the rules ourselves? I suppose it would be better to reuse the already existing rules and toolchain.

@ChALkeR
Copy link
Member

ChALkeR commented Mar 7, 2017

It would be super-cool if someone could continue the remark-based linter approach, fixing the CI support there, and then this new rule introduced here could be added as a plugin to remark.

That would give us an already-existing ruleset that we don't need to re-invent, also that would give us editors support.

@thefourtheye
Copy link
Contributor Author

Closing in favour of #12756 and @ChALkeR's comment above.

@thefourtheye thefourtheye deleted the doc-linter branch April 30, 2017 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants