tools: enable Markdown linter's usage information

[DerekNonGeneric]

Prior to this commit, running node tools/lint-md --help and node tools/lint-md --version resulted in an input error.

• Use unified-args to start processor
• Use unified-args's error handler

Fixes: #30156

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

[DerekNonGeneric]

Can you ping the appropriate people for review?

/to @targos @Xstoudi

[addaleax]

Maybe @Trott ?

[Trott]

Maybe @Trott ?

Sorry, just seeing this now. I've been less-than-perfect about tracking my GitHub notifications lately.

[Trott]

This looks good to me, but when I apply the changes in this PR and then do this:

 ( cd tools/node-lint-md-cli-rollup && npm ci && npm run build-node )

...I end up with a different tools/lint-md.js than in this PR. How did you generate tools/lint-md.js for this PR?

[DerekNonGeneric]

@Trott, I believe the intended way of building this is with make lint-md-rollup (might do exactly what you described).

It seems like the dependencies were updated, which caused the lockfile to change as well. I will update the PR tomorrow (a bit late in the East Coast).

[DerekNonGeneric]

@Trott, I had to change something to trigger a new build (description capitalization). It should be good to go now!

[Trott]

When I run pull in these changes and then run make lint-md-rollup, it results in a different tools/lint-md.js than the one in this PR. I don't think that should happen?

[Trott]

When I run pull in these changes and then run make lint-md-rollup, it results in a different tools/lint-md.js than the one in this PR. I don't think that should happen?

Oh, wait, it does that on master too so that's not an indication of a problem, necessarily.

[Trott]

@RichardLitt Is the unified-args stuff in your wheelhouse by any chance? If so, a review would be great.

@nodejs/linting

[nodejs-github-bot]

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

[DerekNonGeneric]

Something that may be causing these build products to differ is that unified-args depends on chokidar, which has the optional dependency of fsevents, which is only installed for macOS.

I tried building it on Windows 10 (as something different), which resulted in an error. Apparently the @zeit/ncc package doesn't work on Windows. ncc should probably be replaced by rollup (how bundling originally happened, hence the name) if generating a build product on Windows is desired.

[RichardLitt]

I wouldn't have the technical knowledge to help with this, but I think that @wooorm might. I'll ping him.

[wooorm]

I haven’t tested this, but as the author and maintainer of unified-args, this looks good to go! I don’t see any problems.

• Regarding my personal style, I have one inline comment
• I would personally (I’m not a Node maintainer) suggest sticking as close as possible to the remark-cli source code, because it makes picking up changes in future versions easier. Because the ordering is different and names are different, it’s a bit of a hassle

So, personal nits/ideas/opinions only, looks good to me

[DerekNonGeneric]

@woorm, thanks for taking a look at this! Regarding your suggested nit of sticking as close as possible to the remark-cli source code, I'm not sure that would improve future maintainability for a few reasons.

1. How will people know to look at remark-cli for reference?
2. remark-cli's variable names are non-descript and imports are arbitrarily organized.
3. Is there a guarantee that they will stay this way? Is this a convention?

Adding a comment to explain the layout would probably help. Maybe something like the following.

To facilitate future maintenance, this source code is laid out to closely match that of remark-cli.

I'll probably do that while addressing the inline comment you made above.

[wooorm]

1. Yeah, a comment would be useful for the future I think!
3. No guarantee, but remark-cli/cli.js hasn’t changed much in the last four years

[DerekNonGeneric]

Alrighty @wooorm, it should be all set now. It would mean a lot if you could give this your stamp of approval after a final pass. It should be a breeze now that the sources closely match. 🙂

[wooorm]

I think it looks wonderful 😊

[DerekNonGeneric]

Sweet, I signed the commit and added the remaining in-org reviewer (@refack) to the commit message. Hoping to see this make it in before 2020. 🙏

[richardlau]

Sweet, I signed the commit and added the remaining in-org reviewer (@refack) to the commit message. Hoping to see this make it in before 2020. 🙏

It doesn't look like @refack has reviewed this though?

[DerekNonGeneric]

@richardlau, I think we anticipate revision, no? I was under the impression that updating this commit message by someone else in the future (post-approval) would invalidate the signature. To be honest, I'm not sure how these things are handled in this repo and I could probably use some clarification.

I drew the conclusion that it was acceptable to add anticipated reviewers to commit messages because it is a requirement that is checked by core-validate-commit and not having this results in failure. I'm glad this was brought up and I hope someone can dispel my doubts.

[richardlau]

To match what is run on Travis CI, core-validate-commit should be run with --no-validate-metadata.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[Trott]

Landed in 80fb153