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

feat: lint YAML comments in md files #139

Merged
merged 1 commit into from
Oct 16, 2020
Merged

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Sep 30, 2020

Add a linter rule to validate YAML comments in markdown files.

The goal of this PR is to enforce a consistent order convention across the Node.js documentation.

@Trott
Copy link
Member

Trott commented Sep 30, 2020

LGTM if we can get the changes into Node.js core to comply with this. I have opinions on both of the two ordering decisions, but I am much more invested in consistency, so I'm fine with however it ends up.

@aduh95
Copy link
Contributor Author

aduh95 commented Sep 30, 2020

LGTM if we can get the changes into Node.js core to comply with this. I have opinions on both of the two ordering decisions, but I am much more invested in consistency, so I'm fine with however it ends up.

I'm with you on that one, I think consistency is key. Do you want to share your thoughts nonetheless? I think we can achieve consistency with the same amount of work no matter which convention we choose to follow, so if someone wants to weigh in for one option or the other before I start working on it, that'd be awesome.

If I had to come up with rules myself, I'd say we should list versions and changes from the oldest at the top to the newest at the bottom; since most of the docs uses the exact opposite, that's what I'm gonna follow (and I can see why that makes sense so I'm fine with it).

@Trott
Copy link
Member

Trott commented Sep 30, 2020

Do you want to share your thoughts nonetheless?

I'm finding myself changing my mind, so now I'm just going to go with, "I don't care as long as it's consistent."

@aduh95
Copy link
Contributor Author

aduh95 commented Sep 30, 2020

OK, I'll wait for nodejs/node#35191 to land before starting working on PRs on node repo to avoid getting too much git conflicts.

@nschonni
Copy link
Member

Makes sense to me. https://www.npmjs.com/package/semver might be useful if you run into any version comparisons that end up hitting any odd results

@aduh95
Copy link
Contributor Author

aduh95 commented Oct 4, 2020

I wonder if there's a way of checking if a given version number corresponds to a released version of Node.js. That could avoid that kind of mishaps nodejs/node#35454 (comment)

But I don't think there is a reliable way of getting an updated list of Node.js releases, right?

@nschonni
Copy link
Member

nschonni commented Oct 4, 2020

There is the index.json/tab or https://github.com/chicoxyzzy/node-releases which scraps it

@aduh95
Copy link
Contributor Author

aduh95 commented Oct 5, 2020

There is the index.json/tab or https://github.com/chicoxyzzy/node-releases which scraps it

I think that would break at each new release: when the releaser replaces REPLACEME occurrences with the new version number, the test would reject because that version has not yet been released.

@richardlau
Copy link
Member

richardlau commented Oct 5, 2020

There is the index.json/tab or https://github.com/chicoxyzzy/node-releases which scraps it

I think that would break at each new release: when the releaser replaces REPLACEME occurrences with the new version number, the test would reject because that version has not yet been released.

For the release being prepared your only option is going to be scanning the local files -- the releaser should add a changelog entry for the new release. The catch is that the local changelog will only be accurate for the release line being prepared (e.g. if the releaser is preparing a v10.x release only the v10 changelog will be accurate).

index.json/tab or https://github.com/chicoxyzzy/node-releases will be accurate for any releases already made.

@aduh95
Copy link
Contributor Author

aduh95 commented Oct 13, 2020

CI's green 🎉

I've given up the idea of validating version strings to raise flag when an unreleased version number is used; I couldn't find a way to read CHANGELOGs in an elegant way, I think it's fine to rely on the code review process to pick up that kind of mistakes.

I think this is ready to land, please review.

cc/ @Trott @nschonni

Copy link
Member

@nschonni nschonni left a comment

Choose a reason for hiding this comment

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

Not blocking, but the last thought I had on this, is whether it should be sitting in it's own repo, and just referenced here

BethGriggs pushed a commit to nodejs/node that referenced this pull request Oct 14, 2020
Refs: nodejs/remark-preset-lint-node#139

PR-URL: #35454
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BethGriggs pushed a commit to nodejs/node that referenced this pull request Oct 14, 2020
Refs: nodejs/remark-preset-lint-node#139

PR-URL: #35454
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BethGriggs pushed a commit to nodejs/node that referenced this pull request Oct 14, 2020
Refs: nodejs/remark-preset-lint-node#139

PR-URL: #35454
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BethGriggs pushed a commit to nodejs/node that referenced this pull request Oct 14, 2020
Refs: nodejs/remark-preset-lint-node#139

PR-URL: #35454
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this pull request Oct 14, 2020
Refs: nodejs/remark-preset-lint-node#139

PR-URL: #35454
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this pull request Oct 14, 2020
Refs: nodejs/remark-preset-lint-node#139

PR-URL: #35454
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this pull request Oct 14, 2020
Refs: nodejs/remark-preset-lint-node#139

PR-URL: #35575
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ben Coe <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott Trott merged commit e4ddfef into nodejs:master Oct 16, 2020
@Trott
Copy link
Member

Trott commented Oct 16, 2020

Landed and published as 1.17.0, discovered that was bad (left out the new file), and re-published as 1.17.1.

@aduh95 aduh95 deleted the lint-yaml-comments branch October 16, 2020 08:01
Trott added a commit to nodejs/node that referenced this pull request Oct 18, 2020
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
Refs: nodejs/remark-preset-lint-node#139

PR-URL: nodejs#35454
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
Refs: nodejs/remark-preset-lint-node#139

PR-URL: nodejs#35454
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
Refs: nodejs/remark-preset-lint-node#139

PR-URL: nodejs#35454
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
Refs: nodejs/remark-preset-lint-node#139

PR-URL: nodejs#35454
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
Refs: nodejs/remark-preset-lint-node#139

PR-URL: nodejs#35454
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
Refs: nodejs/remark-preset-lint-node#139

PR-URL: nodejs#35454
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
Refs: nodejs/remark-preset-lint-node#139

PR-URL: nodejs#35575
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ben Coe <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit to nodejs/node that referenced this pull request May 1, 2021
Refs: nodejs/remark-preset-lint-node#139

PR-URL: #35454
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit to nodejs/node that referenced this pull request May 1, 2021
danielleadams pushed a commit to nodejs/node that referenced this pull request May 8, 2021
Refs: nodejs/remark-preset-lint-node#139

PR-URL: #35454
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
danielleadams pushed a commit to nodejs/node that referenced this pull request May 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants