-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
tools: introduce make mdlint
command and tools
#12756
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Just one question and one suggestion!
doc/guides/writing-tests.md
Outdated
'sources': [ | ||
'test/cctest/test_env.cc', | ||
... | ||
], | ||
``` | ||
The test can be executed by running the `cctest` target: | ||
``` | ||
```text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn’t this be sh
or bash
or something instead of text
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console
is the correct language for those that start with a $
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated it to console
. It's handy :)
doc/api/v8.md
Outdated
@@ -1,6 +1,6 @@ | |||
# V8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use references and definitions in this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, why not! I've updated it.
@watilde Thanks so much for doing this, I will take a look =). |
doc/api/v8.md
Outdated
[`vm.Script`]: vm.html#vm_new_vm_script_code_options | ||
[here]: https://github.com/thlorenz/v8-flags/blob/master/flags-0.11.md | ||
[`GetHeapSpaceStatistics`]: https://v8docs.nodesource.com/node-5.0/d5/dda/classv8_1_1_isolate.html#ac673576f24fdc7a33378f8f57e1d13a4 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for inlining those?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, there wasn't. I've updated it!
Makefile
Outdated
@@ -960,6 +966,7 @@ endif | |||
lint \ | |||
lint-ci \ | |||
list-gtests \ | |||
mdlint \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Extra indentation.
@@ -853,6 +853,11 @@ bench: bench-net bench-http bench-fs bench-tls | |||
|
|||
bench-ci: bench | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should probably edit the $(TARBALL)
target as well, to exclude the tools/remark*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right! I added it on L717.
This currently fails CI at the linter stage. |
|
@refack If I get it right, |
We need the switch in Also for the reasons I mention in https://github.com/nodejs/node/pull/12744/files#diff-6a3371457528722a734f3c51d9238c13R54 Could you split this into two PRs:
Since I want to review, and I usually use the webUI for that, and 2181 files kills my browser :( P.S. does anyone have a better review tool? |
Linter errors:
|
There should be no need for separate PRs. But yeah, it would be nice to have the changes that only add dependencies in their own commit. |
248a8bb
to
3d6a5c0
Compare
Thanks all for your review comments! I just divided the commit into several commits for the review, and coped with the review comments. For the |
|
Makefile
Outdated
mdlint: | ||
@echo "Running Markdown linter..." | ||
$(NODE) tools/remark-cli/cli.js -q \ | ||
doc/api doc/api_assets doc/guides src lib benchmark tools/doc/ tools/icu/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why are other stuff from doc
is ignored (e.g. onboarding.md
and changelogs)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changelogs should be excluded since they have commit messages that make some lint errors and shouldn't be changed. The other docs can be included, then I will update it with --ignore-path=doc/changelogs
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. There are two lint errors in the whole doc
dir currently:
doc/changelogs/CHANGELOG_V6.md
1325:95-1325: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
I don't agree that the commit messages in the changelog should not be touched — those could be actual errors that made changelog look bad. E.g. if a changelog includes __proto__
, we don't want it to render as «proto», we want to render it as «__proto__
» or «__proto__» — so amending the changelog to escape those things sounds reasonable to me, and I would want the linter to catch those errors. See commit a58b48bc3bc, for example.
So I would prefer if this just replaced handle__
with either `handle__`
or handle\_\_
in the changelogs (and similarly for handle_
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this comment, I thought that anyone should not touch the commit message in the changelog anyway, but it's fine if we've polished it in the past because it's absolutely better for users who read the rendered docs. Will update it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that was about typos, which is pretty much the actual content — it makes sense not to change that.
And the fact that commit messages (initially in text) are converted to changelogs (which are Markdown) without proper escaping is mostly a tooling issue here (which is likely not worth fixing, though — those are very rare and the linter would catch that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that whatever generates the changelogs should have escaped some symbols like `_*~\
, prefixing those with \
. But for the cases that are already there and that require manual fixing, perhaps using inline code blocks makes more sense where appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may also include the root directory as well to check them(e.g. README.md
, CONTRIBUTING.md
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased with your suggestion🐱
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ChALkeR remark is pretty good at escaping and generating markdown btw ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wooorm, I filed an issue to nodejs/changelog-maker#47.
3d6a5c0
to
7498d01
Compare
new ci: https://ci.nodejs.org/job/node-test-pull-request/7772/ |
7498d01
to
fd939da
Compare
The failed test on FreeBSD seems unrelated: https://ci.nodejs.org/job/node-test-commit-freebsd/nodes=freebsd10-64/8681/console |
I'm thinking it would be easier to manage the rule set if I add the preset plugin as just a directory with a |
It's a bit impractical to review the first 2 commits. Last 3 LGTM. |
The I personally don't have anything against that (so no action required atm), but just to be sure: @nodejs/ctc — is that something that we better want in the foundation or not? Edit: /ping @nodejs/tsc |
PR-URL: #12756 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
PR-URL: #12756 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
PR-URL: #12756 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
PR-URL: #12756 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
PR-URL: #12756 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
PR-URL: #12756 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
PR-URL: #12756 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
when this landed it should have come with a license update, without it we would be in violation of the remark-cli MIT license. I've opened #16637 to fix this. |
Should land with #16637 if it lands on v6.x. |
I'd like to figure out what the story is with dependencies before landing this. I think it is safe to add tools in semver-patch, but I don't want to break testing the tarballs |
CONTRIBUTING.md + L857: Unused definition + L861: Unused definition + L863: Unused definition doc/api/assert.md + L719: Unused definition doc/api/async_hooks.md + L460: Missing code-language flag doc/api/child_process.md + L1362: Unused definition doc/api/dns.md + L674: Unused definition doc/api/esm.md + L178: Missing code-language flag doc/api/http.md + L1868: Unused definition + L1887: Unused definition + L1888: Unused definition + L1889: Unused definition + L1916: Unused definition + L1917: Unused definition doc/api/https.md + L260: Unused definition doc/api/os.md + L1226: Unused definition doc/api/process.md + L1888: Unused definition doc/api/stream.md + L2227: Definitions with the same identifier doc/guides/writing-and-running-benchmarks.md + L1: Missing newline character at end of file Refs: nodejs/node#12756 PR-URL: nodejs/node#16385 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: nodejs/node#12756 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
PR-URL: nodejs/node#12756 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
PR-URL: nodejs/node#12756 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
PR-URL: nodejs/node#12756 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
PR-URL: nodejs/node#12756 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
PR-URL: nodejs/node#12756 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
PR-URL: nodejs#14707 Refs: nodejs#12756 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Summary
I'm trying to put this PR as a replacement for #11610 and #8551.
cc @ChALkeR @thefourtheye
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
tools, doc