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: prepare miscellaneous docs for new markdown lint rules #29963

Merged
merged 0 commits into from
Oct 15, 2019

Conversation

Trott
Copy link
Member

@Trott Trott commented Oct 14, 2019

Prepare the final few documents that haven't been updated to always use
[] with reference links and to escape [ and ] for things that
aren't links in markdown files.

These are the final changes required before we can use the next version of remark-preset-lint-node.

@nschonni @nodejs/documentation

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

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. i18n-api Issues and PRs related to the i18n implementation. tools Issues and PRs related to the tools directory. labels Oct 14, 2019
@Trott
Copy link
Member Author

Trott commented Oct 14, 2019

Collaborators, please 👍 here to fast-track.

@Trott
Copy link
Member Author

Trott commented Oct 14, 2019

@Trott Trott added the fast-track PRs that do not need to wait for 48 hours to land. label Oct 14, 2019
* `buf` [<Buffer>]
* return [<ArrayBufferView[]>]
* `buf` [<Buffer>][]
* return [<ArrayBufferView>][]\[\]
Copy link
Member

Choose a reason for hiding this comment

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

Think the reference link might need to be updated instead for the 3 in this section to include the array modifier in the identifier

Copy link
Member

Choose a reason for hiding this comment

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

The current way does work though

Copy link
Member Author

Choose a reason for hiding this comment

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

My thinking in preferring this way is that the linked info is not about ArrayBufferView[]. It's about ArrayBufferView. So the link text reflects that.

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 14, 2019
@XhmikosR
Copy link
Contributor

XhmikosR commented Oct 14, 2019

Escaping the square brackets is not needed AFAICT. Which tool complains?

EDIT: For example, this shows just fine without escaping [ https://github.com/nodejs/node/tree/master/benchmark#createbenchmarkfn-configs-options

@Trott
Copy link
Member Author

Trott commented Oct 14, 2019

Escaping the square brackets is not needed AFAICT. Which tool complains?

EDIT: For example, this shows just fine without escaping [ https://github.com/nodejs/node/tree/master/benchmark#createbenchmarkfn-configs-options

It makes the text a link (without displaying brackets at all) if the text happens to match a bottom-reference, which it did in one case that @nschonni found. So it only causes problems infrequently. Or at least that's my understanding. Maybe @nschonni can confirm/clarify.

@nschonni
Copy link
Member

The instance this did flag correctly was #29809 (comment) and the rule that flags it is https://github.com/remarkjs/remark-lint/tree/master/packages/remark-lint-no-undefined-references

@Trott
Copy link
Member Author

Trott commented Oct 14, 2019

The instance this did flag correctly was #29809 (comment) and the rule that flags it is https://github.com/remarkjs/remark-lint/tree/master/packages/remark-lint-no-undefined-references

And in the current version of the docs (for 12.12.0), you can see the problem: https://nodejs.org/dist/latest-v12.x/docs/api/crypto.html#crypto_diffiehellman_generatekeys_encoding

The brackets are gone and "encoding" is now link text.

@nschonni
Copy link
Member

From the upstream build failure, there are a few new ones in doc/api/vm.md

tmp/doc/api/vm.md
  315:28-315:45  warning  Found reference to undefined definition               no-undefined-references     remark-lint
  565:46-565:57  warning  Use the trailing [] on reference links                no-shortcut-reference-link  remark-lint
  565:46-565:57  warning  Found reference to undefined definition               no-undefined-references     remark-lint
  656:70-656:81  warning  Use the trailing [] on reference links                no-shortcut-reference-link  remark-lint
  656:70-656:81  warning  Found reference to undefined definition               no-undefined-references     remark-lint

@Trott
Copy link
Member Author

Trott commented Oct 15, 2019

From the upstream build failure, there are a few new ones in doc/api/vm.md

tmp/doc/api/vm.md
  315:28-315:45  warning  Found reference to undefined definition               no-undefined-references     remark-lint
  565:46-565:57  warning  Use the trailing [] on reference links                no-shortcut-reference-link  remark-lint
  565:46-565:57  warning  Found reference to undefined definition               no-undefined-references     remark-lint
  656:70-656:81  warning  Use the trailing [] on reference links                no-shortcut-reference-link  remark-lint
  656:70-656:81  warning  Found reference to undefined definition               no-undefined-references     remark-lint

Good to know. I'm OK with fixing those up when the lint rule lands on master, just as long as there's three lines to fix and not 150.

@XhmikosR
Copy link
Contributor

XhmikosR commented Oct 15, 2019

It's no big deal, I'm just trying to understand why perfectly fine square brackets are flagged. Are the docs being built here on CI? Is the output the same?

@Trott
Copy link
Member Author

Trott commented Oct 15, 2019

It's no big deal, I'm just trying to understand why perfectly fine square brackets are flagged. Are the docs being built here on CI? Is the output the same?

Docs are built on CI so that if any changes to the docs happen that break doctool or whatever, we know. I don't think that output is checked, but I don't have any reason to believe it's different than when people run locally.

@Trott
Copy link
Member Author

Trott commented Oct 15, 2019

Landed in ed5eaa0

@Trott Trott closed this Oct 15, 2019
@Trott Trott merged commit ed5eaa0 into nodejs:master Oct 15, 2019
targos pushed a commit that referenced this pull request Nov 8, 2019
Prepare the final few documents that haven't been updated to always use
`[]` with reference links and to escape `[` and `]` for things that
aren't links in markdown files.

PR-URL: #29963
Reviewed-By: Masashi Hirano <shisama07@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
targos pushed a commit that referenced this pull request Nov 10, 2019
Prepare the final few documents that haven't been updated to always use
`[]` with reference links and to escape `[` and `]` for things that
aren't links in markdown files.

PR-URL: #29963
Reviewed-By: Masashi Hirano <shisama07@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@Trott Trott deleted the update-md-linting branch January 13, 2022 22:52
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. fast-track PRs that do not need to wait for 48 hours to land. i18n-api Issues and PRs related to the i18n implementation. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants