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

tools: enable Markdown linter's usage information #30216

Closed
wants to merge 1 commit into from
Closed

tools: enable Markdown linter's usage information #30216

wants to merge 1 commit into from

Conversation

DerekNonGeneric
Copy link
Contributor

@DerekNonGeneric DerekNonGeneric commented Nov 2, 2019

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

Fixes: #30156

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

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Nov 2, 2019
@DerekNonGeneric
Copy link
Contributor Author

Can you ping the appropriate people for review?

/to @targos @Xstoudi

@addaleax
Copy link
Member

Maybe @Trott ?

@Trott
Copy link
Member

Trott commented Dec 6, 2019

Maybe @Trott ?

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

@Trott
Copy link
Member

Trott commented Dec 6, 2019

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
Copy link
Contributor Author

DerekNonGeneric commented Dec 6, 2019

@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
Copy link
Contributor Author

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

@Trott
Copy link
Member

Trott commented Dec 7, 2019

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
Copy link
Member

Trott commented Dec 7, 2019

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 Trott requested a review from refack December 7, 2019 05:05
@Trott
Copy link
Member

Trott commented Dec 7, 2019

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

@nodejs/linting

@nodejs-github-bot
Copy link
Collaborator

@DerekNonGeneric
Copy link
Contributor Author

DerekNonGeneric commented Dec 7, 2019

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
Copy link
Contributor

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

Copy link

@wooorm wooorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

tools/node-lint-md-cli-rollup/src/cli-entry.js Outdated Show resolved Hide resolved
@DerekNonGeneric
Copy link
Contributor Author

@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.

Copy link

@wooorm wooorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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. 🙂

Copy link

@wooorm wooorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it looks wonderful 😊

@DerekNonGeneric
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

DerekNonGeneric commented Dec 12, 2019

@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.

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

* Use `unified-args` to start processor
* Use `unified-args`'s error handler

Fixes: #30156

PR-URL: #30216
@richardlau
Copy link
Member

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

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Dec 15, 2019

Landed in 80fb153

@Trott Trott closed this Dec 15, 2019
Trott pushed a commit that referenced this pull request Dec 15, 2019
Prior to this commit, running `node tools/lint-md --help` and
`node tools/lint-md --version` resulted in an error.

* Use `unified-args` to start processor
* Use `unified-args`'s error handler

Fixes: #30156
PR-URL: #30216
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
Prior to this commit, running `node tools/lint-md --help` and
`node tools/lint-md --version` resulted in an error.

* Use `unified-args` to start processor
* Use `unified-args`'s error handler

Fixes: #30156
PR-URL: #30216
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 17, 2019
targos pushed a commit that referenced this pull request Jan 14, 2020
Prior to this commit, running `node tools/lint-md --help` and
`node tools/lint-md --version` resulted in an error.

* Use `unified-args` to start processor
* Use `unified-args`'s error handler

Fixes: #30156
PR-URL: #30216
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
Prior to this commit, running `node tools/lint-md --help` and
`node tools/lint-md --version` resulted in an error.

* Use `unified-args` to start processor
* Use `unified-args`'s error handler

Fixes: #30156
PR-URL: #30216
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tools: Markdown lint script's --help does not work as advertised
7 participants