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

doc: use .md extension for internal links #35191

Merged
merged 3 commits into from
Oct 1, 2020
Merged

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Sep 14, 2020

This helps catch broken links as part of the test suite. This also
improves the user experience when browsing the markdown files.

Fixes: #35189

This is currently blocked by #35182.

Checklist
  • documentation is changed or added
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Sep 14, 2020
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/http2
  • @nodejs/modules
  • @nodejs/n-api
  • @nodejs/net

@DerekNonGeneric DerekNonGeneric added the blocked PRs that are blocked by other issues or PRs. label Sep 14, 2020
doc/api/crypto.md Outdated Show resolved Hide resolved
doc/api/n-api.md Outdated Show resolved Hide resolved
@aduh95 aduh95 marked this pull request as ready for review September 14, 2020 20:38
@aduh95 aduh95 requested review from a team as code owners September 14, 2020 20:38
@aduh95 aduh95 requested a review from a team September 14, 2020 20:38
@DerekNonGeneric DerekNonGeneric added tools Issues and PRs related to the tools directory. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed blocked PRs that are blocked by other issues or PRs. labels Sep 14, 2020
@aduh95
Copy link
Contributor Author

aduh95 commented Sep 16, 2020

Yikes, there are already conflicts… I'll resolve them as soon as I have a moment.

@DerekNonGeneric, would you kindly ping @nodejs/documentation as well? Their input would be valuable given the size of the change.

@DerekNonGeneric
Copy link
Contributor

Oh hmm, the bot should have done that. Wish we had that awaiting feedback label that was proposed recently.

/cc @nodejs/documentation

@DerekNonGeneric DerekNonGeneric requested a review from a team September 16, 2020 12:13
@gengjiawen
Copy link
Member

@aduh95 You need to rebase.

@Trott
Copy link
Member

Trott commented Sep 16, 2020

Since my changes caused the conflicts, I resolved them and force-pushed.

@Trott
Copy link
Member

Trott commented Sep 16, 2020

Oh hmm, the bot should have done that.

If the bot pinged the documentation team on changes to doc files, they'd probably be pinged on around half of the PRs in the repo. (Same for the testing team.) Since the auto-pinging is a new thing, I'm guessing the idea has been to roll it out slowly and not overwhelm large-ish teams with pings.

@aduh95
Copy link
Contributor Author

aduh95 commented Sep 22, 2020

Rebased and split into 3 commits because #35224 landed before this one. PTAL

@aduh95 aduh95 force-pushed the use-md-extension branch 2 times, most recently from 45e287d to c8d000b Compare September 26, 2020 12:10
Copy link
Contributor

@DerekNonGeneric DerekNonGeneric left a comment

Choose a reason for hiding this comment

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

This looks good overall. I just have a couple of questions.

};

function replaceLinks({ filename, linksMapper }) {
return (tree) => {
const fileHtmlUrls = linksMapper[filename];

visit(tree, (node) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you link to this API's docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have just copied the code below, I haven't used the API docs nor do I know where to find it.

Copy link
Contributor

Choose a reason for hiding this comment

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

From what I was able to find out, unist-util-visit uses the Universal Syntax Tree.

@wooorm might be able to clue us in on whether there are additional learning resources, but so far this code seems to be fully functional to me. It would be nice to get a better understanding of how this works though.

Copy link

Choose a reason for hiding this comment

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

readme for unist-util-visit(-parents) should have enough info. There are also some recipes on the website of unified.

tools/doc/markdown.js Show resolved Hide resolved
tools/doc/markdown.js Show resolved Hide resolved
@@ -0,0 +1,47 @@
'use strict';
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the command you ran to make this test run?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

make test-doc

node/Makefile

Line 606 in 7390098

$(PYTHON) tools/test.py $(PARALLEL_ARGS) doctool; \

tools/doc/html.js Show resolved Hide resolved
const gtocMD = fs.readFileSync(gtocPath, 'utf8').replace(/^<!--.*?-->/gms, '');
const gtocMD = fs.readFileSync(gtocPath, 'utf8')
.replace(/\(([^#?]+?)\.md\)/ig, (_, filename) => `(${filename}.html)`)
.replace(/^<!--.*?-->/gms, '');
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you recall what this replacement was for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To remove HTML comments it seems. I haven't looked into it though.

tools/doc/markdown.js Outdated Show resolved Hide resolved
};

function replaceLinks({ filename, linksMapper }) {
return (tree) => {
const fileHtmlUrls = linksMapper[filename];

visit(tree, (node) => {
if (node.url) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to know what this data structure looks like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's the result of a console.log(node) here:

{
  type: 'link',
  title: null,
  url: '#cli_diagnostic_dir_directory',
  children: [ { type: 'text', value: '--diagnostic-dir', position: [Position] } ],
  position: Position {
    start: { line: 131, column: 1, offset: 3405 },
    end: { line: 131, column: 50, offset: 3454 },
    indent: []
  }
}

@aduh95 aduh95 force-pushed the use-md-extension branch 2 times, most recently from 6388632 to 844ed1b Compare September 29, 2020 08:15
@aduh95
Copy link
Contributor Author

aduh95 commented Sep 29, 2020

Rebased to solve conflict. If everyone is happy with the current code, I'd like to have it landed before other conflicts arise.

@DerekNonGeneric my understanding is that none of your comments are blocking, is that right?

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2020
@nodejs-github-bot
Copy link
Collaborator

This helps catch broken links as part of the test suite. This also
improves the user experience when browsing the markdown files.

PR-URL: nodejs#35191
Fixes: nodejs#35189
Reviewed-By: Derek Lewis <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Refs: nodejs#35244

PR-URL: nodejs#35191
Fixes: nodejs#35189
Reviewed-By: Derek Lewis <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Fixes: nodejs#35189

PR-URL: nodejs#35191
Reviewed-By: Derek Lewis <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@Trott
Copy link
Member

Trott commented Oct 1, 2020

Landed in 726143e...91837e9

@Trott Trott merged commit 91837e9 into nodejs:master Oct 1, 2020
@aduh95 aduh95 deleted the use-md-extension branch October 1, 2020 13:38
danielleadams pushed a commit that referenced this pull request Oct 6, 2020
This helps catch broken links as part of the test suite. This also
improves the user experience when browsing the markdown files.

PR-URL: #35191
Fixes: #35189
Reviewed-By: Derek Lewis <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
danielleadams pushed a commit that referenced this pull request Oct 6, 2020
Refs: #35244

PR-URL: #35191
Fixes: #35189
Reviewed-By: Derek Lewis <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
danielleadams pushed a commit that referenced this pull request Oct 6, 2020
Fixes: #35189

PR-URL: #35191
Reviewed-By: Derek Lewis <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@danielleadams danielleadams mentioned this pull request Oct 6, 2020
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
This helps catch broken links as part of the test suite. This also
improves the user experience when browsing the markdown files.

PR-URL: nodejs#35191
Fixes: nodejs#35189
Reviewed-By: Derek Lewis <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
Refs: nodejs#35244

PR-URL: nodejs#35191
Fixes: nodejs#35189
Reviewed-By: Derek Lewis <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
Fixes: nodejs#35189

PR-URL: nodejs#35191
Reviewed-By: Derek Lewis <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

doc,tools: checkLinks.js does not cover api
10 participants