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

docs: set markdown tabwidth: 4 #30608

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

SnakeDoc
Copy link
Contributor

@SnakeDoc SnakeDoc commented Aug 6, 2024

Changes

PR as requested in #30569

Sets .prettierrc.json and .editorconfig markdown config to use 4 space characters as tab width.
pnpm doc-fix-everything was run after the tab width changes, resulting in most or all markdown files being updated.

Context

See discussion in #30569 regarding a possible markdown formatting conflict between prettier and mkdocs. This PR is to see what changing markdown tabWidth: 4 would impact.

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@SnakeDoc
Copy link
Contributor Author

SnakeDoc commented Aug 6, 2024

the linter got me - working on a fix

@SnakeDoc
Copy link
Contributor Author

SnakeDoc commented Aug 6, 2024

tagging another manager with the same prettier issue in #30569 - https://docs.renovatebot.com/modules/manager/conan/

.prettierrc.json Outdated Show resolved Hide resolved
@SnakeDoc
Copy link
Contributor Author

SnakeDoc commented Aug 6, 2024

The linter issue will need to be addressed in .markdownlint-cli2.jsonc. Docs point at this workaround, however DavidAnson/markdownlint#138 looks like it will still cause issues for the original problem nested list structure.

@SnakeDoc
Copy link
Contributor Author

SnakeDoc commented Aug 6, 2024

the following .markdownlink-cli2.jsonc config is recommended when using markdownlint with Prettier:

"list-marker-space": {
  "ul_multi": 3,
  "ul_single": 3
},
"ul-indent": {
  "indent": 4
}
which produces this output:
markdownlint-cli2 v0.13.0 (markdownlint v0.34.0)
Finding: **/*.md !**/__fixtures__/* !**/node_modules/** !./tmp/**/*
Linting: 264 file(s)
Summary: 24 error(s)
docs/development/minimal-reproductions.md:43:5 MD030/list-marker-space Spaces after list markers [Expected: 3; Actual: 1]
docs/development/minimal-reproductions.md:44:5 MD030/list-marker-space Spaces after list markers [Expected: 3; Actual: 1]
docs/development/minimal-reproductions.md:45:5 MD030/list-marker-space Spaces after list markers [Expected: 3; Actual: 1]
docs/usage/getting-started/migrating-secrets.md:69:5 MD030/list-marker-space Spaces after list markers [Expected: 3; Actual: 1]
docs/usage/getting-started/migrating-secrets.md:70:5 MD030/list-marker-space Spaces after list markers [Expected: 3; Actual: 1]
docs/usage/getting-started/migrating-secrets.md:71:5 MD030/list-marker-space Spaces after list markers [Expected: 3; Actual: 1]
docs/usage/key-concepts/changelogs.md:59:5 MD030/list-marker-space Spaces after list markers [Expected: 3; Actual: 1]
docs/usage/key-concepts/changelogs.md:60:5 MD030/list-marker-space Spaces after list markers [Expected: 3; Actual: 1]
docs/usage/key-concepts/changelogs.md:61:5 MD030/list-marker-space Spaces after list markers [Expected: 3; Actual: 1]
docs/usage/key-concepts/changelogs.md:63:5 MD030/list-marker-space Spaces after list markers [Expected: 3; Actual: 1]
docs/usage/key-concepts/changelogs.md:64:5 MD030/list-marker-space Spaces after list markers [Expected: 3; Actual: 1]
docs/usage/key-concepts/changelogs.md:66:5 MD030/list-marker-space Spaces after list markers [Expected: 3; Actual: 1]
docs/usage/key-concepts/changelogs.md:67:5 MD030/list-marker-space Spaces after list markers [Expected: 3; Actual: 1]
docs/usage/key-concepts/changelogs.md:69:5 MD030/list-marker-space Spaces after list markers [Expected: 3; Actual: 1]
docs/usage/nuget.md:32:5 MD030/list-marker-space Spaces after list markers [Expected: 3; Actual: 1]
docs/usage/nuget.md:33:5 MD030/list-marker-space Spaces after list markers [Expected: 3; Actual: 1]
lib/modules/manager/conan/readme.md:12:5 MD030/list-marker-space Spaces after list markers [Expected: 3; Actual: 1]
lib/modules/manager/conan/readme.md:13:5 MD030/list-marker-space Spaces after list markers [Expected: 3; Actual: 1]
lib/modules/manager/conan/readme.md:14:5 MD030/list-marker-space Spaces after list markers [Expected: 3; Actual: 1]
lib/modules/manager/gleam/readme.md:19:5 MD030/list-marker-space Spaces after list markers [Expected: 3; Actual: 1]
lib/modules/manager/gleam/readme.md:21:5 MD030/list-marker-space Spaces after list markers [Expected: 3; Actual: 1]
lib/modules/manager/gleam/readme.md:23:5 MD030/list-marker-space Spaces after list markers [Expected: 3; Actual: 1]
lib/modules/platform/codecommit/readme.md:22:5 MD030/list-marker-space Spaces after list markers [Expected: 3; Actual: 1]
lib/modules/platform/codecommit/readme.md:23:5 MD030/list-marker-space Spaces after list markers [Expected: 3; Actual: 1]
 ELIFECYCLE  Command failed with exit code 1.

If we disable the two configs like shown here:

"list-marker-space": false,
"ul-indent": false
we get the following instead:
markdownlint-cli2 v0.13.0 (markdownlint v0.34.0)
Finding: **/*.md !**/__fixtures__/* !**/node_modules/** !./tmp/**/*
Linting: 264 file(s)
Summary: 10 error(s)
docs/usage/index.md:66:1 MD030/list-marker-space Spaces after list markers [Expected: 1; Actual: 3]
docs/usage/index.md:67:1 MD030/list-marker-space Spaces after list markers [Expected: 1; Actual: 3]
docs/usage/index.md:68:1 MD030/list-marker-space Spaces after list markers [Expected: 1; Actual: 3]
docs/usage/index.md:69:1 MD030/list-marker-space Spaces after list markers [Expected: 1; Actual: 3]
docs/usage/index.md:70:1 MD030/list-marker-space Spaces after list markers [Expected: 1; Actual: 3]
docs/usage/index.md:71:1 MD030/list-marker-space Spaces after list markers [Expected: 1; Actual: 3]
docs/usage/index.md:72:1 MD030/list-marker-space Spaces after list markers [Expected: 1; Actual: 3]
docs/usage/index.md:73:1 MD030/list-marker-space Spaces after list markers [Expected: 1; Actual: 3]
docs/usage/index.md:111:1 MD030/list-marker-space Spaces after list markers [Expected: 1; Actual: 3]
docs/usage/index.md:112:1 MD030/list-marker-space Spaces after list markers [Expected: 1; Actual: 3]
 ELIFECYCLE  Command failed with exit code 1.

Removing https://github.com/renovatebot/renovate/blob/main/docs/usage/index.md?plain=1#L60 and re-running pnpm lint with the same disabled rules:

markdownlint-cli2 v0.13.0 (markdownlint v0.34.0)
Finding: **/*.md !**/__fixtures__/* !**/node_modules/** !./tmp/**/*
Linting: 264 file(s)
Summary: 0 error(s)

However, this means these two list rules won't be evaluated.

@SnakeDoc
Copy link
Contributor Author

SnakeDoc commented Aug 6, 2024

tagging our resident docs expert, @HonkingGoose, for an opinion/advice on these linter rules.

@HonkingGoose
Copy link
Collaborator

First impressions/thoughts:

  • I don't like the extra spaces after the list items
  • I don't like ignoring linter rules, the tests/linter should catch as much as possible
  • It's hard for me to confirm if these changes fix the bad indentation in the published docs

@SnakeDoc
Copy link
Contributor Author

SnakeDoc commented Aug 8, 2024

  • I don't like the extra spaces after the list items

It was odd/unexpected for me as well. However it does appear to be a style opinion aimed at increasing readability for root-level lists. markdownlint discusses this here and makes a reference to this markdown style guide. Prettier discusses the same thing here. It does this only for root-level lists that contain content - including embedded lists, like identified in #30569.

  • I don't like ignoring linter rules, the tests/linter should catch as much as possible

I agree.

  • It's hard for me to confirm if these changes fix the bad indentation in the published docs

Unfortunately, it's complicated. My original opinion was we should leave things be the way they are, since the risk of making such a sweeping change (and possibly causing or uncovering more rendering/formatting issues) to fix a relatively benign problem seemed overkill. "Let sleeping dogs lie". However, it seems 4 space tab width is actually the more correct tab width for markdown in general, and perhaps this change should be made regardless.

This drafted PR makes the change to 4 space tab width, but has made the linter very angry in the process. To solve the linter issue, markdownlint provides guidance on using prettier alongside linting, specifically in relation to lists.

However, from what I can tell, it appears that markdownlint does not currently understand the nested list structure that was identified as a problem in #30569. So, this PR does not appear to address the issue #30569 discovered.

I think this leaves us with some options:

  1. Disable markdownlint list-marker-space checking (probably undesirable)
  2. Make the proposed changes, and re-write all nested list structures to something else
  3. Look for a way to disable/alter this list space issue in prettier
  4. ??? something else?

2 - Simple enough to fix now, but is there a way to automatically enforce this? As-in ban the structure or something? This feels limiting for doc writers.

3 - I have not been successful finding a way to adjust this behavior in prettier directly. I have only found ways to make the linter ignore this issue.

FWIW, markdownlint maintains a list of "rules that might conflict with prettier", which disables many checks including the problematic list spacing rules. I understand it's not desirable to disable so many rules, but it might be notable that the maintainer of markdownlint hints these rules might be trouble.

tldr; Every option has other problems from what I can tell. I could be missing something though. Thoughts from anyone?

@HonkingGoose
Copy link
Collaborator

Others have similar problems

I searched the Material for MkDocs discussions. Others are having similar problems too:

Maintainer for Material for MkDocs formats manually

The maintainer of Material for MkDocs formats everything by hand. 1 That's okay, when you write most of the docs, and review all PRs. But we get many contributions, and I can't check them all. 😄 So we should keep using linters to format our code/docs.

About your bullet items

  1. Disable markdownlint list-marker-space checking (probably undesirable)
  2. Make the proposed changes, and re-write all nested list structures to something else
  3. Look for a way to disable/alter this list space issue in prettier
  4. ??? something else?
  1. I don't like globally disabling linter rules, unless that's the actual fix.
  2. This could work, but manually enforcing a certain style is easy to forget, and leads to friction for PR authors.
  3. If we could get Prettier to do the right thing, that would be best.
  4. I don't have any better ideas, for now.

TLDR

tldr; Every option has other problems from what I can tell. I could be missing something though. Thoughts from anyone?

Yeah, all the solutions are good/bad in different ways. Maybe someone else has the perfect solution. 🙃

Footnotes

  1. Material for MkDocs maintainer formats manually

@HonkingGoose
Copy link
Collaborator

I found this discussion:

The maintainer of Material for MkDocs (@squidfunk) says to use four spaces per tab for Python Markdown.

It's a bit scary making such a big change in settings. 🙂

@SnakeDoc
Copy link
Contributor Author

Hmm, this comment in particular is unfortunate.

It seems we're in agreement 4 space tab width is correct for Markdown, but the list issue is pretty ugly with the current linter + prettier. I still have not found a way to adjust the prettier list-marker-space setting or similar, although it's quite possible I've simply overlooked it. I'm not as familiar with prettier as I would like to be, unfortunately.

🤔

@viceice
Copy link
Member

viceice commented Aug 16, 2024

at least you can preview your changes now

  1. run pdm install (once)
  2. run pnpm mkdocs serve

@rarkins
Copy link
Collaborator

rarkins commented Sep 22, 2024

@SnakeDoc thank you for attempting this fix, although I'm not sure if it's something we plan to move forward with? Could you summarize the latest status/plans?

@SnakeDoc
Copy link
Contributor Author

Thanks @rarkins.

The original issue we were attempting to fix was identified in #30569 - nested markdown lists. Caused by a tabwidth conflict - mkdocs requiring 4 space tabs, and renovate using the prettier default of 2 space tabs.

It does appear 4 space tabs are more correct for markdown - however that change presently causes new problems that don't appear easily mitigated.

Specifically, changing to 4 space tabs results in a conflict between prettier and markdownlint, which have different opinions on how nested lists should be formatted. The only workaround I've found is to disable some of markdownlint's rules - which is undesirable. It is possible I overlooked something, however.

To be clear - the conflict between prettier and markdownlint is specifically for nested markdown lists (list inside a list); normal lists (ordered and unordered) are not a problem.

In #30857 we added a prettier-ignore directive + comment linking to this PR on all known occurrences of nested lists, which mitigates this issue for the time-being. However, there is nothing in place that prevents a future contributor from re-introducing the problematic nested list structure.

I see three possibilities:

  1. Proceed with changing to 4 space tabs - and leave in-place prettier-ignore on nested lists.
  2. Close this PR and leave the tabwidth at the prettier default of 2 spaces, also leaving prettier-ignore on nested lists.
  3. Leave this PR open and hope something will come up in the future that helps resolve this.

I am unsure how to proceed on this, or even if we should proceed at this point.

@rarkins
Copy link
Collaborator

rarkins commented Sep 27, 2024

Is there another possibility that we could leave the source files in this repo as prettier's default, but then add a "massage" function as part of our docs build to detect and modify nested lists prior to passing to mkdocs?

@HonkingGoose
Copy link
Collaborator

I think Prettier is unlikely to add support for 4-spaces as tab-width, as they say they follow the CommonMark spec:

But we could try to create a PR to support 4-spaces in ordered lists in the Markdownlint program? The maintainer says they're open to a PR:

So we have a variant of option 1:

  • Variant option 1: proceed with changing to 4 space tabs - and leave in-place prettier-ignore on nested lists. Open PR to add support for 4 spaces as tabs to Markdown linter.

@HonkingGoose
Copy link
Collaborator

Is there another possibility that we could leave the source files in this repo as prettier's default, but then add a "massage" function as part of our docs build to detect and modify nested lists prior to passing to mkdocs?

I find it hard to understand how we transform the input into the final docs build. Adding more steps to the build makes understanding the build even more difficult.

For me, adding a massage step seems worse than fixing our source docs. I know we're a bit stuck on figuring out the best way to fix the source docs... 😄

If the maintainers decide that a massage step is the best way forward, that's fine by me too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants