doc: harmonize version pattern in YAML comments by aduh95 · Pull Request #35575 · nodejs/node · GitHub

I've let two linter errors slipped through in #35454.

Refs: nodejs/remark-preset-lint-node#139

Checklist

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

Fast-track? (So we can land the linting rule for this stuff so @aduh95 doesn't have to open a PR like this ever three days?)

I've also included some changes so we can check PR-URLs as well – and I did catch some more typos.

Probably less of a sure-thing fast-track candidate with those additional changes, but I'm still good to fast-track it if someone else agrees.

Not sure I follow why some sections are getting disabled with the second commit

Why disable the in this block?

That’s because one item in the changes list has been introduced without a PR on node repo for security reason (3948830). This is (will be) picked up by the linter as invalid (see https://github.com/nodejs/remark-preset-lint-node/runs/1235246278?check_suite_focus=true)

Ideally the linter should be aware of that pattern, but I figured disabling linter on the few comments it happens would be good enough on a first iteration.

Is the pattern that if the pr-url is for nodejs-private/node-private, then it should have a matching commit on frontmatter?

OK, I agree that’s a cleaner solution. I didn’t take the time to implement that on the linter PR, I’ll try to do that later today.

No worries, just talking the problem out loud 😄

Should the commit be a URL as well?
Should the commit be disallowed on non-private pr-urls?

Should the commit be a URL as well?

Before I started working on refactoring YAML comments, there were 14 occurrences of commit IDs:

13 were commit: sha
1 was pr-url: https://github.com/nodejs/node/commit/sha

I'd stick with the majority here, as we did on the previous PR, unless someone objects.

Should the commit be disallowed on non-private pr-urls?

Yes, the goal is consistency, let's avoid it where we can. My plan is to have it allowed only on changes introduced on Node.js 0.x (because sometimes changes were made directly on master), and on security releases.

Should the dual pr-url be allowed in the future, or better to ignore the one-off?

I think that's a one off, we can revisite later if it becomes more common.

Removed the fast-track as now the PR is open for more than 48 hours anyway.

Just want to explicitly say:

I'm comfortable with fast tracking this

And, I'm excited to see nodejs/remark-preset-lint-node#139 land 👍

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

@cjihrig @Trott @lpinca, could I bother folks for an explicit 👍 for a fast-track?

Note: looks like we're about to hit the 72 hour weekend guideline any ways, but better to be explicit.

Refs: nodejs/remark-preset-lint-node#139 PR-URL: nodejs#35575 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Landed in 0aa2c5b

Note: looks like we're about to hit the 72 hour weekend guideline any ways, but better to be explicit.

FYI, and I know it's hard to keep track and that it's not relevant for this PR anymore, but good news: There is no 72 hour weekend rule anymore. It's 48 hours no matter when you open the PR. Please don't use that fact to game the system. :-D

Refs: nodejs/remark-preset-lint-node#139 PR-URL: #35575 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Refs: nodejs/remark-preset-lint-node#139 PR-URL: nodejs#35575 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Oct 9, 2020

aduh95 mentioned this pull request Oct 9, 2020

feat: lint YAML comments in md files nodejs/remark-preset-lint-node#139

Merged

nschonni approved these changes Oct 9, 2020

View reviewed changes

Trott approved these changes Oct 9, 2020

View reviewed changes

aduh95 added the fast-track PRs that do not need to wait for 48 hours to land. label Oct 9, 2020

aduh95 force-pushed the harmonize-yaml-comments branch from 81cd64c to ac66a68 Compare October 10, 2020 11:01

aduh95 requested a review from Trott October 10, 2020 11:06

Trott approved these changes Oct 10, 2020

View reviewed changes

nschonni reviewed Oct 10, 2020

View reviewed changes

aduh95 force-pushed the harmonize-yaml-comments branch from ac66a68 to 465ed49 Compare October 10, 2020 18:35

nschonni reviewed Oct 10, 2020

View reviewed changes

aduh95 added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. and removed fast-track PRs that do not need to wait for 48 hours to land. labels Oct 11, 2020

aduh95 force-pushed the harmonize-yaml-comments branch from 465ed49 to 72c088e Compare October 12, 2020 17:39

bcoe approved these changes Oct 12, 2020

View reviewed changes

bcoe mentioned this pull request Oct 12, 2020

doc: fix REPLACEME typo #35617

Closed

3 tasks

lpinca approved these changes Oct 12, 2020

View reviewed changes

bcoe added the fast-track PRs that do not need to wait for 48 hours to land. label Oct 12, 2020

jasnell approved these changes Oct 13, 2020

View reviewed changes

doc: harmonize YAML comments

0aa2c5b

Refs: nodejs/remark-preset-lint-node#139 PR-URL: nodejs#35575 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

aduh95 force-pushed the harmonize-yaml-comments branch from 72c088e to 0aa2c5b Compare October 13, 2020 13:14

aduh95 merged commit 0aa2c5b into nodejs:master Oct 13, 2020

MylesBorins mentioned this pull request Oct 14, 2020

v14.14.0 proposal #35648

Merged

aduh95 deleted the harmonize-yaml-comments branch July 16, 2022 16:31

doc: harmonize version pattern in YAML comments by aduh95 · Pull Request #35575 · nodejs/node · GitHub

Uh oh!

Uh oh!

doc: harmonize version pattern in YAML comments - #35575

doc: harmonize version pattern in YAML comments#35575aduh95 merged 1 commit intonodejs:masternodejs/node:masterfrom aduh95:harmonize-yaml-commentsaduh95/node:harmonize-yaml-commentsCopy head branch name to clipboard

Conversation

aduh95 commented Oct 9, 2020

Checklist

Uh oh!

Trott commented Oct 9, 2020

Uh oh!

aduh95 commented Oct 10, 2020

Uh oh!

Trott commented Oct 10, 2020

Uh oh!

nschonni left a comment

Choose a reason for hiding this comment

Uh oh!

nschonni Oct 10, 2020

Choose a reason for hiding this comment

Uh oh!

aduh95 Oct 10, 2020

Choose a reason for hiding this comment

Uh oh!

nschonni Oct 10, 2020

Choose a reason for hiding this comment

Uh oh!

aduh95 Oct 10, 2020

Choose a reason for hiding this comment

Uh oh!

nschonni Oct 10, 2020

Choose a reason for hiding this comment

Uh oh!

aduh95 Oct 10, 2020

Choose a reason for hiding this comment

Uh oh!

nschonni Oct 10, 2020

Choose a reason for hiding this comment

Uh oh!

aduh95 Oct 10, 2020 • 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!

aduh95 commented Oct 11, 2020

Uh oh!

bcoe commented Oct 12, 2020

Uh oh!

nodejs-github-bot commented Oct 12, 2020

Uh oh!

bcoe commented Oct 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduh95 commented Oct 13, 2020

Uh oh!

Trott commented Oct 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

doc: harmonize version pattern in YAML comments#35575
aduh95 merged 1 commit into
nodejs:masterfrom
aduh95:harmonize-yaml-comments

aduh95 Oct 10, 2020 •
edited
Loading

bcoe commented Oct 12, 2020 •
edited
Loading