Inconsistent formatting of Markdown files

[scinos]

Problem

Our Markdown files are inconsistent. Some examples (not an exhaustive list)

• apps/o2-blocks/README.md uses hashes to create H1 tiles, while apps/notifications/README.md uses underline marks

• apps/notifications/README.md uses - to create lists, while CODE-OF-CONDUCT.md uses *

• Code blocks have syntax errors (docs/data-persistence.md)

• Code blocks without language declaration (docs/icons.md)

• Mixture of regular spaces and non-breaking spaces and other typographical utf8 marks (docs/testing/testing-overview.md)

• Mixture of spaces and tabs for indentation (apps/full-site-editing/full-site-editing-plugin/global-styles/README-DATA.md)

• Inconsistent line lengths (docs/data-persistence.md vs docs/development-workflow.md)

Solution

We can treat Markdown files as any other file with code, and use linters to fix the problems described above. We can even use the existing stack of linters (eslint + prettier) to provide a developer experience very similar to linting JS/TS files.

• eslint will be the main lint engine for Markdown files. This allows using estlint from the command line to lint MD files (eslint --ext .md <file.md>), re-use the existing estlint integration for editors (eg: highlight MD errors as we type, fix them on save), and keep custom linting rules together (.eslintrc will contain rules for JS, TS and MD files).

• prettier already has support for Markdown, it provides a set of opinionated defaults for Markdown. As with JS/TS files, prettier rules will be the main source of formatting rules (i.e. will disable eslint formatting rules that conflict with prettier rules)

• remark is the internal parser used by eslint to read and format Markdown files. It incorporate their own set of rules (https://github.com/remarkjs/remark-lint/tree/master/packages/remark-preset-lint-markdown-style-guide#rules). We will not enable them for now. Once all our MD files are linted following prettier rules, we'll revisit this.

Current status

The current error count is:

✖ 6688 problems (6688 errors, 0 warnings)
  6636 errors and 0 warnings potentially fixable with the `--fix` option.

Most of them are auto-fixable. Note that this does not include the fix for the tabs vs spaces (see Note 1), so the actual error count may be lower and the ratio of fixable errors may be higher.

The number of affected files is 816. Their distribution is:

    4 /
    1 apps/
    5 apps/full-site-editing
    6 apps/notifications
    1 apps/wpcom-block-editor
    1 client/
    1 client/assets
   66 client/blocks
    1 client/boot
  238 client/components
    1 client/config
    1 client/controller
    5 client/devdocs
   56 client/extensions
    1 client/gutenberg
    5 client/landing
   10 client/layout
   97 client/lib
   30 client/me
   75 client/my-sites
    1 client/notices
    5 client/post-editor
   14 client/reader
    6 client/server
    1 client/signup
   51 client/state
    1 config/
   26 docs/
    7 docs/guide
    4 docs/testing
    6 docs/coding-guidelines
    1 packages/
    1 packages/babel-plugin-i18n-calypso
    1 packages/babel-plugin-transform-wpcalypso-async
    1 packages/calypso-analytics
    3 packages/calypso-build
    1 packages/calypso-codemods
    1 packages/calypso-color-schemes
    1 packages/calypso-polyfills
   11 packages/components
    1 packages/composite-checkout-wpcom
    1 packages/composite-checkout
    3 packages/data-stores
    1 packages/effective-module-tree
    2 packages/eslint-config-wpcalypso
   15 packages/eslint-plugin-wpcalypso
    2 packages/i18n-calypso-cli
    2 packages/i18n-calypso
    1 packages/load-script
    1 packages/material-design-icons
    1 packages/media-library
    2 packages/photon
    1 packages/tree-select
    2 packages/viewport-react
    2 packages/viewport
    1 packages/webpack-config-flag-plugin
    1 packages/webpack-extensive-lodash-replacement-plugin
    1 packages/webpack-inline-constant-exports-plugin
    2 packages/wpcom-proxy-request
   15 packages/wpcom.js
    1 test/
    8 test/e2e
    2 test/test

Execution status

Phase 1: contention

The goal is to stop introducing new lint errors.

• Merge prettier plugin for eslint. This will also affect JS files. See Adds prettier plugin to eslint #40118
• Upstream fix for Sublists aren't parsed when indented with tabs remarkjs/remark#198. (In progress)
• Migrate tabs to spaces in all MD files (see Note 1). This is a pre-requisite for the next step. See Reindent markdon with spaces as a workaround for remarkjs/remark#198 #40798 (no needed with the upstream fix)
• Merge the upstream fix in remark to prettier to get a new version (likely 2.0.5)
• Update and release wp-prettier with the same remark update and match prettier's version
• Enable markdown linting in eslint. This will allow developers to lint the MD files they are working on, and cause a CI fail if changed MD files are not linted correctly.

Phase 2: reduction

The goal is to reduce the number of existing errors to zero.

The strategy will be to progressively fix existing errors, starting with folders with low number of errors and ramp up once we are confident in the changes that prettier will make and we get more experience with the kind of problems to look for

Phase 3: expansion

Experiment with further rules we may want to apply to our current codebase. For example, enable remark rules. This phase will also include create a custom MD rule that developers can use as a starting point to create future rules.

Notes

1. Due a bug in remark (Sublists aren't parsed when indented with tabs remarkjs/remark#198), the parser used by prettier, we'll need to use spaces to indent lists. Once the issue linked above is fixed, we can revisit and use tabs.

[sarayourfriend]

Getting prettier onto the latest remark-parse version is going to be a significant project. There's some discussion in this issue prettier/prettier#6180 about moving to v6 from about a year ago. Can we help by putting some time towards assisting prettier in being able to upgrade? Changing remark-parse to the latest version locally causes a bunch of tests to fail in prettier, at a glance it doesn't look like it's going to be straightforward by any means.

In the meantime, do we have a workaround that could unblock us for the time being? It seems like all but the last item in phase 1 won't happen until Prettier's dependency is updated, but if we move forward with spaces for markdown files like you suggested @scinos, will that be okay?

[scinos]

Thanks for checking. I didn't know prettier was so behind in terms of remark.

There is an interesting plot twist worth mentioning: we are not using prettier, but our own fork wp-prettier. The idea was to update prettier to use latest remark, and then update our fork, so they are in sync.

But we don't really have to update prettier first, we could just update our fork with latest remark (which means they will start to drift away). I don't think this is a great idea, but it is worth mentioning.

I'd go with using spaces in our Markdown for now.

[sarayourfriend]

we could just update our fork with latest remark

I tried updating remark in prettier proper and around 80 markdown snapshot tests fail in non-obvious ways 😞 remark 5 -> 6 was a big one, it turns out and the loose/spread change discussed in the prettier issue I linked (it's actually discussed in a handful of issues) seems to have been pretty significant. If we got it working in wp-prettier we might as well just open a PR into prettier to fix everything; I think it would largely be the same amount and type of work.

My vote is to just use spaces as well 🙂

[sarayourfriend]

The PR to update remark in prettier: prettier/prettier#8140 It seems like the PR should be mergeable soon 🎉

[github-actions]

This issue is stale because it has been 180 days with no activity. You can keep the issue open by adding a comment. If you do, please provide additional context and explain why you’d like it to remain open. You can also close the issue yourself — if you do, please add a brief explanation and apply one of relevant issue close labels.

[scinos]

Already fixed in trunk