Introduce 'make doclint' command and tooling#8551

ChALkeR · 2016-09-15T19:22:54Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

tools

Description of change

Note: there were some more changes in this PR, those were extracted to #8660, #8553, #8666, and were merged already.

Add remark-cli and remark-lint to tools/remark-cli, introducing new make mdlint command.

make mdlint now lints all docs, make test now executes make mdlint.

A complete make mdlint check takes 8-9 seconds on my notebook.

Previous work in #7729.

/cc @Trott, @jasnell, @addaleax.

Trott · 2016-09-15T20:21:31Z

Should there be a vcbuild.bat mdlint for Windows users?

ChALkeR · 2016-09-17T18:55:15Z

Rebased. @Trott, I don't think I would be able to test vcbuild.bat on my PC, so perhaps it would be better if someone with a Windows setup made it.

Trott · 2016-09-18T20:43:02Z

@ChALkeR I don't have direct access to Windows either, but in the past, if it's just one or two lines, I've updated vcbuild.bat and then posted in the GitHub tracker asking someone on Windows to try it out, and that usually works. (Nothing wrong with your approach either! Just suggesting another option.)

/cc @nodejs/platform-windows

addaleax · 2016-09-20T11:55:08Z

This should probably get all the dont-land-on-v?.x labels because this is an almost purely additional change and we’re not eager to backport the doc cleanups, right? I’m setting them but feel free to remove if you think that’s not a good idea.

Also, did you check whether the dependencies here end up in the distribution tarballs?

ChALkeR · 2016-09-20T13:22:16Z

@addaleax Thanks, I didn't think of that.

I will split this into two separate PRs — the actual rules update that would allow me to file more pull requests with fixes without conflicts in the config file, and the make mdlint tooling that would probably consist of separate commits that I would squash on landing.

jasnell · 2016-09-20T19:07:19Z

Awesome! I won't sign off on this just yet as I haven't had a chance to review but I'm very happy to see this.

jbergstroem · 2016-09-23T12:07:59Z

Is it possible to get a -ci representation that spits out tap13?

wooorm · 2016-09-23T14:12:50Z

Thanks for using remark, folks! 🎉
Please ping me if something’s broken / want my advice or input on anything!

Trott · 2016-09-23T16:44:12Z

I'm OK with this landing without a vcbuild implementation or a doclint-ci job just as long as we open separate issues for those things (and maybe tag them help wanted and/or good first contribution as appropriate).

Rubber-stamp LGTM if CI is fine (which I imagine is really just going to make sure that the .eslintignore stuff is working and that this doesn't accidentally break the Makefile).

This adds remark-lint tooling and introduces a 'make doclint' command, which is also executed on 'make test' runs. tools/remark-cli dir was created by installing `remark-cli` and `remark-lint`, the following files and directories are being excluded: * `./node_modules/concat-stream/node_modules/readable-stream/` a duplicate copy of `readable-stream` of 2.0, there already is 2.1. * `./**/doc/`, `./**/test/` -- docs and tests for deps are not needed. * `./**/history.md` and `./**/changelog.md` (case-insensitive). * `./**/.travis.yml`, `./**/.istanbul.yml`, `./**/.zuul.yml`. * `./**/.eslintrc`, `./**/.jscs.json`, `./**/.jshintrc`. * `./**/.npmignore`, `./**/component.json`, `./**/.gitattributes`. * `./**/dist/` (affects `sprintf-js` and `js-yaml`), not `require()`-d. PR-URL: nodejs#8551

ChALkeR · 2016-09-25T10:15:27Z

Renamed to make doclint.

Todo:

Make sure this tooling doesn't get packed in the release tarball.
vcbuild.bat support.
doclint-ci

ChALkeR · 2016-10-26T16:04:19Z

This got stalled because of the CI support, I'm planning to:

postpone the CI integration to a separate PR,
postpone the vcbuild.bat support, as I currently can't test on Windows,
fix the other issues here soon.

silverwind · 2016-11-09T17:30:54Z

Makefile

 $(MAKE) cctest
 $(PYTHON) tools/test.py --mode=release -J \
 addons doctool inspector known_issues message pseudo-tty parallel sequential
+$(MAKE) doclint


Maybe rename to mdlint? I assume we want to lint every md, including non-doc files.

The thing is, there is a project named mdlint, and that's not the one we are using.

I don't see that as an issue. There is a project called jslint and we happily use that name for js lint task 😉.

silverwind · 2016-11-09T17:31:58Z

I'd suggest you split this in 2 commits, one for the module, one for config changes, so it's easier to review.

silverwind · 2016-11-09T18:58:09Z

I'd suggest running dmn clean -f before committing those modules to save a few bytes:

remark-cli: OK: Done! Your node_modules directory size was 2860.10 Kb but now it's 2594.57 Kb which is 9.3% less. remark-lint: OK: Done! Your node_modules directory size was 388.04 Kb but now it's 362.54 Kb which is 6.6% less.

eljefedelrodeodeljefe · 2016-11-09T19:29:29Z

I find this to be unnecessary to be included into the tree. it's a lot of files and only adds value for linting. Would a npm install remark-lint@specific-version && ./node_modules/.bin/remark-linter not be enough? Also given the shaky nature of markdown processor development.

silverwind · 2016-11-09T19:42:22Z

I'm not sure we can require a working npm to run tests. It'd certainly complicate the build process for distribution package maintainers.

eljefedelrodeodeljefe · 2016-11-09T19:44:41Z

well it is possible if we don't run then through make and adopt practical Node testing behaviour. One could say this would require Node then, but it is available as binary or you just take the compiled one.

Trott · 2016-11-09T22:26:55Z

well it is possible if we don't run then through make and adopt practical Node testing behaviour.

Since we include eslint already, there's precedence for including large lint tools as dependencies in the tree. I can't think of a reason to handle eslint and remark differently.

If we want to change it for both, then that's a pretty big change with a lot to consider. (For example: Do we want our CI and release stuff to fail if the npm registry is down? I don't know the answer and this is not the only thing we'd need to consider.)

It may be a great idea--I have no opinion, at least not at this time--but I would prefer that such a change should be a separate PR so that just that practice (bundling the tools with our repository vs. installing them over the network with tools) is up for discussion. It would be a shame for a meta-issue like that (no matter how important) to stall this.

eljefedelrodeodeljefe · 2016-11-09T22:36:03Z

Well I think eslint is not comparable since it actually lints the actual language we are defining here. So that would functionally be more important than a Markup language that could be get rid of any time. That being said, yes they should be treated equally namely getting rid of eslint too.

There are more than checking in and installing as possibilities for CI. E.g. imaginable would be global dependencies on the node that get provisioned with the Node or a static Docker container.
Also our releases and builds are not time critical and they can be down for a while as does the rest of user and application land.

I wouldn't merge this until the meta issue is resolved. It's also a little funny that there is no meta issue yet, since we are seeing remark commits since the mid this year, always with hints "let's rather not rely on that tool". This was foreseeable.

eljefedelrodeodeljefe · 2016-11-09T22:40:14Z

edit: also writing an external tool that could also include remark as dependency would be thinkable.

jasnell · 2017-01-06T20:17:10Z

@ChALkeR ... next steps on this?

jasnell · 2017-03-01T01:56:08Z

Closing this in favor of #11610

This adds remark-lint tooling and introduces a 'make doclint' command, which is also executed on 'make test' runs. tools/remark-cli dir was created by installing `remark-cli` and `remark-lint`, the following files and directories are being excluded: * `./node_modules/concat-stream/node_modules/readable-stream/` a duplicate copy of `readable-stream` of 2.0, there already is 2.1. * `./**/doc/`, `./**/test/` -- docs and tests for deps are not needed. * `./**/history.md` and `./**/changelog.md` (case-insensitive). * `./**/.travis.yml`, `./**/.istanbul.yml`, `./**/.zuul.yml`. * `./**/.eslintrc`, `./**/.jscs.json`, `./**/.jshintrc`. * `./**/.npmignore`, `./**/component.json`, `./**/.gitattributes`. * `./**/dist/` (affects `sprintf-js` and `js-yaml`), not `require()`-d. PR-URL: nodejs#8551

ChALkeR · 2017-03-07T20:29:09Z

@jasnell, I still think that this should be done through using an already existing tool and rules, possibly extending the rules, if needed, and not writing and supporting our own markdown linter ruleset and toolset from the scratch.

I will probably file another remark-based PR from the same branch (can't reopen this one) after the Buffer issues get sorted out.

Alternatively, anyone else is free to take off based on https://github.com/ChALkeR/io.js/tree/remark, the main issue there was the missing TAP test results for the CI. 😉

Perhaps the remark modules need to be updated since then, too =).

ChALkeR added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Sep 15, 2016

nodejs-github-bot added build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. labels Sep 15, 2016

ChALkeR force-pushed the remark branch from 35191b0 to 2dde684Compare September 15, 2016 19:33

ChALkeR mentioned this pull request Sep 15, 2016
doc: fix a formatting error in buffer.md #8553
Merged
2 tasks

ChALkeR force-pushed the remark branch from 2dde684 to ea1b9a9Compare September 17, 2016 18:53

ChALkeR force-pushed the remark branch from ea1b9a9 to a55c295Compare September 20, 2016 04:05

ChALkeR mentioned this pull request Sep 20, 2016
tools: clean up icu/README.md formatting #8660
Merged
2 tasks

addaleax added dont-land-on-v4.x labels Sep 20, 2016

ChALkeR force-pushed the remark branch from a55c295 to 0497c60Compare September 20, 2016 13:15

ChALkeR mentioned this pull request Sep 20, 2016
tools: update remark configuration #8666
Merged
2 tasks

Trott force-pushed the master branch from b0df363 to c5ce7f4Compare September 21, 2016 00:09

ChALkeR force-pushed the remark branch from 0497c60 to f5cd8f5Compare September 21, 2016 18:04

ChALkeR force-pushed the remark branch from f5cd8f5 to 30e6d01Compare September 25, 2016 10:13

ChALkeR changed the title ~~Introduce 'make mdlint' command and tooling~~Introduce 'make doclint' command and toolingSep 25, 2016

ChALkeR force-pushed the remark branch from 30e6d01 to da76dc5Compare September 25, 2016 10:13

ChALkeR mentioned this pull request Oct 4, 2016
fs: undeprecate existsSync #8364
Closed
3 tasks

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88Compare October 18, 2016 17:02

ChALkeR mentioned this pull request Nov 9, 2016
tools: add remark-lint configuration in .remarkrc #7729
Merged
1 task

silverwind reviewed Nov 9, 2016
View reviewed changes

Trott mentioned this pull request Mar 1, 2017
introduce doc linter #11610
Closed
2 tasks

jasnell closed this Mar 1, 2017

watilde mentioned this pull request Apr 30, 2017
tools: introduce make mdlint command and tools #12756
Closed
2 tasks

Uh oh!

Introduce 'make doclint' command and tooling#8551

Introduce 'make doclint' command and tooling #8551

Uh oh!

Conversation

ChALkeR commented Sep 15, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Trott commented Sep 15, 2016

Uh oh!

ChALkeR commented Sep 17, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Sep 18, 2016

Uh oh!

addaleax commented Sep 20, 2016

Uh oh!

ChALkeR commented Sep 20, 2016

Uh oh!

jasnell commented Sep 20, 2016

Uh oh!

jbergstroem commented Sep 23, 2016

Uh oh!

wooorm commented Sep 23, 2016

Uh oh!

Trott commented Sep 23, 2016

Uh oh!

ChALkeR commented Sep 25, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChALkeR commented Oct 26, 2016

Uh oh!

silverwindNov 9, 2016

Choose a reason for hiding this comment

Uh oh!

ChALkeRNov 9, 2016

Choose a reason for hiding this comment

Uh oh!

silverwindNov 9, 2016• 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!

silverwind commented Nov 9, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

silverwind commented Nov 9, 2016

Uh oh!

eljefedelrodeodeljefe commented Nov 9, 2016

Uh oh!

silverwind commented Nov 9, 2016

Uh oh!

eljefedelrodeodeljefe commented Nov 9, 2016

Uh oh!

Trott commented Nov 9, 2016

Uh oh!

eljefedelrodeodeljefe commented Nov 9, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eljefedelrodeodeljefe commented Nov 9, 2016

Uh oh!

jasnell commented Jan 6, 2017

Uh oh!

jasnell commented Mar 1, 2017

Uh oh!

ChALkeR commented Mar 7, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

ChALkeR commented Sep 15, 2016•
edited
Loading

ChALkeR commented Sep 17, 2016•
edited
Loading

ChALkeR commented Sep 25, 2016•
edited
Loading

silverwindNov 9, 2016•
edited
Loading

silverwind commented Nov 9, 2016•
edited
Loading

eljefedelrodeodeljefe commented Nov 9, 2016•
edited
Loading

ChALkeR commented Mar 7, 2017•
edited
Loading