Skip to content

Conversation

@aduh95
Copy link
Contributor

@aduh95aduh95 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-botnodejs-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

@aduh95aduh95 marked this pull request as ready for review September 14, 2020 20:38
@aduh95aduh95 requested review from a team as code ownersSeptember 14, 2020 20:38
@aduh95aduh95 requested a review from a teamSeptember 14, 2020 20:38
@aduh95
Copy link
ContributorAuthor

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

@gengjiawen
Copy link
Member

@aduh95 You need to rebase.

@Trott
Copy link
Member

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

@Trott
Copy link
Member

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.

Copy link
Member

@richardlaurichardlau left a comment

Choose a reason for hiding this comment

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

I'm not exactly sure what needs to change, but I had a quick look at the docs generated by this PR and it looks like the links in the left section now point to the *.md files.
image

image

You can get a built copy of the docs from the "misc / build-docs" check from this PR:
image

@aduh95aduh95 requested review from richardlau and removed request for a teamSeptember 18, 2020 07:41
@richardlaurichardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 18, 2020
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 18, 2020
@aduh95aduh95 mentioned this pull request Sep 22, 2020
4 tasks
@aduh95
Copy link
ContributorAuthor

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

@aduh95aduh95force-pushed the use-md-extension branch 2 times, most recently from 45e287d to c8d000bCompareSeptember 26, 2020 12:10
Copy link
Contributor

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

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
ContributorAuthor

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.

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.

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
ContributorAuthor

Choose a reason for hiding this comment

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

make test-doc

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

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
ContributorAuthor

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.

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
ContributorAuthor

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: []}}

@aduh95aduh95force-pushed the use-md-extension branch 2 times, most recently from 6388632 to 844ed1bCompareSeptember 29, 2020 08:15
@aduh95
Copy link
ContributorAuthor

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?

@TrottTrott added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2020
@github-actionsgithub-actionsbot 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#35191Fixes: 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#35191Fixes: 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

@TrottTrott merged commit 91837e9 into nodejs:masterOct 1, 2020
@aduh95aduh95 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: #35191Fixes: #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: #35191Fixes: #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]>
@danielleadamsdanielleadams 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#35191Fixes: 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#35191Fixes: 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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.docIssues and PRs related to the documentations.toolsIssues 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

@aduh95@nodejs-github-bot@DerekNonGeneric@gengjiawen@Trott@wooorm@lpinca@richardlau@aymen94@trivikr