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: consistent unordered list style #29516

Closed

Conversation

nschonni
Copy link
Member

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • 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. meta Issues and PRs related to the general management of the project. labels Sep 10, 2019
@nschonni nschonni force-pushed the doc--consistent-unordered-list-style branch 2 times, most recently from bf53da5 to ad5de3b Compare September 10, 2019 06:07
@trivikr
Copy link
Member

trivikr commented Sep 11, 2019

Is there a preference for using * over -?

Prettier, a popular code formatter uses - over * as shows below:

> cat > test.md
Test
* One
* Two       
^C
> npx prettier test.md
Test

- One
- Two

@nschonni
Copy link
Member Author

I actually prefer - 😄 , I was just going by what looked like the more used format

@trivikr
Copy link
Member

trivikr commented Sep 11, 2019

I was just going by what looked like the more used format

Some results from grep in node repo:

> grep -r --include=\*.md '^* ' ./ | wc -l
   46352
> grep -r --include=\*.md '^\- ' ./ | wc -l
    6176

* is definitely more popular than - for first level of indentation

@trivikr
Copy link
Member

trivikr commented Sep 11, 2019

@trivikr trivikr added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 11, 2019
@nschonni
Copy link
Member Author

@Trott Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 12, 2019
doc/api/n-api.md Outdated Show resolved Hide resolved
@Trott
Copy link
Member

Trott commented Sep 12, 2019

This PR accidentally altered YAML front-matter, breaking make doc.

It seems like an awfully big change set (1943 changed lines in its current form) and there might be other issues in there like this. I kind of doubt anyone actually reviewed all the changes here and instead rubber-stamped it. I'm not sure we should do it. It seems like we're doing it to be consistent for the sake of being consistent, which isn't terrible or anything, but it doesn't seem that using * in one file and - in another file is actually causing any issues for anyone. (Correction on that welcome.)

If it's possible to set the linter so that it requires one or the other in each file, but rejecting a mixture of both in a single file, that might be an acceptable first step towards eventually making them all consistent.

I'm not going to block this but I'm kind of -0 on it.

doc/api/n-api.md Outdated Show resolved Hide resolved
doc/api/n-api.md Outdated Show resolved Hide resolved
doc/api/n-api.md Outdated Show resolved Hide resolved
doc/api/process.md Outdated Show resolved Hide resolved
doc/api/process.md Outdated Show resolved Hide resolved
@nschonni
Copy link
Member Author

nschonni commented Sep 12, 2019

This PR accidentally altered YAML front-matter, breaking make doc.

Thanks, I'll take a look at that

If it's possible to set the linter so that it requires one or the other in each file, but rejecting a mixture of both in a single file, that might be an acceptable first step towards eventually making them all consistent.

Most of the changed files were because they were mixed

it doesn't seem that using * in one file and - in another file is actually causing any issues for anyone. (Correction on that welcome.)

Nope, different across docs is fine. The only issue that can really arise from mixed is if they are accidentally used at the same level, then multiple unorded lists are created. Can't recall if I actually saw any in here

@nschonni nschonni force-pushed the doc--consistent-unordered-list-style branch from 007a547 to d15fd5d Compare September 13, 2019 00:50
@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Sep 13, 2019

make doc still failing because this PR still modifies YAML in a way that makes it invalid.

doc/api/crypto.md Outdated Show resolved Hide resolved
doc/api/crypto.md Outdated Show resolved Hide resolved
@nschonni nschonni force-pushed the doc--consistent-unordered-list-style branch from d15fd5d to 2d86d5c Compare September 13, 2019 04:22
@nodejs-github-bot
Copy link
Collaborator

@nschonni nschonni force-pushed the doc--consistent-unordered-list-style branch from 2d86d5c to 2db627e Compare September 13, 2019 05:39
@nodejs-github-bot
Copy link
Collaborator

Convert to astericks when there are mixed styles in document.
Addresses Markdownlint MD004 rule
@nschonni nschonni force-pushed the doc--consistent-unordered-list-style branch from dab098f to f2bd82f Compare September 14, 2019 05:31
@Trott
Copy link
Member

Trott commented Sep 14, 2019

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

Trott commented Sep 16, 2019

Landed in e2dcbf1

Trott pushed a commit that referenced this pull request Sep 16, 2019
Convert to asterisks when there are mixed styles in document.
Addresses Markdownlint MD004 rule

PR-URL: #29516
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@Trott Trott closed this Sep 16, 2019
@nschonni nschonni deleted the doc--consistent-unordered-list-style branch September 16, 2019 18:34
@nschonni
Copy link
Member Author

Thanks, I'll look at adding that to the remark-lint config if you don't get to it first

targos pushed a commit that referenced this pull request Sep 20, 2019
Convert to asterisks when there are mixed styles in document.
Addresses Markdownlint MD004 rule

PR-URL: #29516
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Sep 24, 2019
BridgeAR pushed a commit that referenced this pull request Sep 25, 2019
Convert to asterisks when there are mixed styles in document.
Addresses Markdownlint MD004 rule

PR-URL: #29516
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Feb 17, 2020
tniessen pushed a commit that referenced this pull request Feb 23, 2020
Erroneously removed in #29516.

Fixes: #31810
Refs: #29516

PR-URL: #31825
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
codebytere pushed a commit that referenced this pull request Feb 27, 2020
Erroneously removed in #29516.

Fixes: #31810
Refs: #29516

PR-URL: #31825
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
codebytere pushed a commit that referenced this pull request Mar 15, 2020
Erroneously removed in #29516.

Fixes: #31810
Refs: #29516

PR-URL: #31825
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
Erroneously removed in #29516.

Fixes: #31810
Refs: #29516

PR-URL: #31825
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
codebytere pushed a commit that referenced this pull request Mar 30, 2020
Erroneously removed in #29516.

Fixes: #31810
Refs: #29516

PR-URL: #31825
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
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. meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants