doc: harmonize version pattern in YAML comments #35575

aduh95 · 2020-10-09T19:11:39Z

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

Trott · 2020-10-09T20:33:20Z

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?)

aduh95 · 2020-10-10T11:06:31Z

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

Trott · 2020-10-10T12:49:58Z

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.

nschonni

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

nschonni · 2020-10-10T14:23:20Z

doc/api/http2.md

@@ -2059,13 +2059,17 @@ The socket timeout logic is set up on connection, so changing this
 value only affects new connections to the server, not any existing connections.

 ### `http2.createServer(options[, onRequestHandler])`
+
+<!--lint disable nodejs-yaml-comments -->


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.

nschonni · 2020-10-10T20:58:16Z

doc/api/deprecations.md

@@ -2143,6 +2143,9 @@ The `crypto._toBuf()` function was not designed to be used by modules outside
 of Node.js core and was removed.

 ### DEP0115: `crypto.prng()`, `crypto.pseudoRandomBytes()`, `crypto.rng()`
+
+<!--lint disable nodejs-yaml-comments -->


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.

aduh95 · 2020-10-11T21:42:34Z

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

bcoe · 2020-10-12T18:00:01Z

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 👍

nodejs-github-bot · 2020-10-12T18:53:18Z

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

bcoe · 2020-10-12T19:17:19Z

@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 <[email protected]> Reviewed-By: Ben Coe <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>

aduh95 · 2020-10-13T13:14:40Z

Landed in 0aa2c5b

Trott · 2020-10-13T18:52:15Z

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 <[email protected]> Reviewed-By: Ben Coe <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>

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]>

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 pending requests for changes, 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 <[email protected]> Reviewed-By: Ben Coe <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: harmonize version pattern in YAML comments #35575

doc: harmonize version pattern in YAML comments #35575

aduh95 commented Oct 9, 2020

Trott commented Oct 9, 2020

aduh95 commented Oct 10, 2020

Trott commented Oct 10, 2020

nschonni left a comment

nschonni Oct 10, 2020

aduh95 Oct 10, 2020

nschonni Oct 10, 2020

aduh95 Oct 10, 2020

nschonni Oct 10, 2020

aduh95 Oct 10, 2020

nschonni Oct 10, 2020

aduh95 Oct 10, 2020 •

edited

Loading

aduh95 commented Oct 11, 2020

bcoe commented Oct 12, 2020

nodejs-github-bot commented Oct 12, 2020

bcoe commented Oct 12, 2020 •

edited

Loading

aduh95 commented Oct 13, 2020

Trott commented Oct 13, 2020

doc: harmonize version pattern in YAML comments #35575

doc: harmonize version pattern in YAML comments #35575

Conversation

aduh95 commented Oct 9, 2020

Checklist

Trott commented Oct 9, 2020

aduh95 commented Oct 10, 2020

Trott commented Oct 10, 2020

nschonni left a comment

Choose a reason for hiding this comment

nschonni Oct 10, 2020

Choose a reason for hiding this comment

aduh95 Oct 10, 2020

Choose a reason for hiding this comment

nschonni Oct 10, 2020

Choose a reason for hiding this comment

aduh95 Oct 10, 2020

Choose a reason for hiding this comment

nschonni Oct 10, 2020

Choose a reason for hiding this comment

aduh95 Oct 10, 2020

Choose a reason for hiding this comment

nschonni Oct 10, 2020

Choose a reason for hiding this comment

aduh95 Oct 10, 2020 • edited Loading

Choose a reason for hiding this comment

aduh95 commented Oct 11, 2020

bcoe commented Oct 12, 2020

nodejs-github-bot commented Oct 12, 2020

bcoe commented Oct 12, 2020 • edited Loading

aduh95 commented Oct 13, 2020

Trott commented Oct 13, 2020

aduh95 Oct 10, 2020 •

edited

Loading

bcoe commented Oct 12, 2020 •

edited

Loading