doc: Add markdownlint rules & apply formatting

ch4ot1c commented at 11:44 pm on September 17, 2019: contributor

This PR introduces a rules file called .markdownlint.yml, and a sizeable amount of documentation formatting from having applied these rules.

.markdownlint.yml can be used by running npm package markdownlint from the repo root. In this case, I ran:

0markdownlint . --ignore "doc/release-notes*" --ignore "doc/release-notes/*" --ignore "src/leveldb/*" --ignore "src/leveldb/**/*" --ignore "src/univalue/*" --ignore "src/univalue/**/*" --ignore "src/secp256k1/*" --ignore "src/secp256k1/**/*" --ignore "src/crypto/**/*" --ignore "ci/retry/*"

A few advantages come from adding this linter:

Consistent formatting and improved readability, both rendered and as plaintext.
.markdownlint.yml automatically offers warnings in VSCode if plugin markdownlint installed.
Could be used to lint Doxygen comments.

fanquake added the label Docs on Sep 17, 2019

fanquake added the label Needs Conceptual Review on Sep 17, 2019

meshcollider commented at 11:56 pm on September 17, 2019: contributor

-0, I think this is a little unnecessary

DrahtBot commented at 1:38 am on September 18, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16960 (doc: replace outdated OpenSSL comment in test README by fanquake)
#16953 (doc: Improve test READMEs by fjahr)
#16797 (scripts: Add convenience script for committing scripted-diffs from a file by ch4ot1c)
#16392 (WIP build: macOS toolchain update by fanquake)
#15459 (doc: add how to calculate blockchain and chainstate size variables to release process by marcoagner)
#15441 ([doc] build: warn against spaces in working directory by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in ci/retry/README.md:1 in a48f402460 outdated

0@@ -1,9 +1,8 @@
1-retry - The command line retry tool
2-------------------------------------------
3+# retry - The command line retry tool

fanquake commented at 4:27 am on September 18, 2019:

This is upstream code. Please don’t modify it here.

in doc/developer-notes.md:5 in a48f402460 outdated

1@@ -2,41 +2,47 @@ Developer Notes
2 ===============
3 
4 <!-- markdown-toc start -->
5+<!-- markdownlint-disable no-emphasis-as-header -->

fanquake commented at 4:33 am on September 18, 2019:

I wouldn’t be in favor of having to turn markdownlint warnings on and off throughout files. It’s going to just look messy non-rendered (not everyone reads our documentation on GitHub). It also seems to go against the whole point of choosing and enforcing some sort of formatting.

ch4ot1c commented at 8:34 am on September 18, 2019:

Agreed

fanquake commented at 4:57 am on September 18, 2019: member

Thanks. However I’m also ~0 on this. While I agree in general that it’d nice to have consistent formatting etc throughout the documentation. Going as far as someone needing to known the markdown “rules” before they can actually write any docs doesn’t seem ideal.

I’m also a bit confused, because it looks like even with the reformatting & linting, there is still inconsistency? i.e using ============ and ### variants for headers. As well as code blocks and indentation for inline code. Is this meant to be the case?

Will wait for other opinions.

This linter could be added to Travis CI, failing if it generates any output.

Definitely NACK on this. To be honest it’s bad enough that test runs for new PRs can sometimes be blocked with whitespace issues. I don’t really want them to be blocked because someone forgot to add a # to a doc header or similar.

If npm is a concerning dependency

I don’t really want us to be relying on or installing npm for anything.

.markdownlint.yml automatically offers warnings in VSCode

We don’t really pick/support any particular editor. I’m not sure what the VSCode usage (specifically for writing markdown?) is like among contributors; but I’d guess it’s not very large (could be wrong).

ch4ot1c commented at 8:35 am on September 18, 2019: contributor

=== is an h1 or #, and --- is an h2 or ##. MD003 is false, so they can be mixed within a file. Since unordered lists are now consistent within a file, and it looks weird, I could see a good argument for changing that.

The code blocks can be made both ways, correct.

Thanks for the feedback on Travis. I thought about it all a bit more, and would be happy to omit .markdownlint.yml and any devtools stuff from the pr; it can just be a set of changes. Keeps things clean, no npm.

Two other notes:

“Upgrading LevelDB” and a few other sections weren’t previously visible in the Table of Contents. This is fixed as part of these changes.
Files like CONTRIBUTING.md may eventually be distributed as just CONTRIBUTING. In this case, sections like the ordered list in “Contributor Workflow” would be best written as 1. 2. 3. instead of 1. 1. 1..

MarcoFalke commented at 10:15 am on September 18, 2019: member

I am fine on adding the yml file, but NACK on enforcing anything or reformatting existing code. We have .style.yapf and .clang-format, but their use is not enforced.

promag commented at 11:29 am on September 18, 2019: member

Concept ACK on improving markdown.

I also don’t mind the markdown linter.

laanwj commented at 12:22 pm on September 18, 2019: member

Thanks. However I’m also ~0 on this. While I agree in general that it’d nice to have consistent formatting etc throughout the documentation. Going as far as someone needing to known the markdown “rules” before they can actually write any docs doesn’t seem ideal.

Completely agree with this. Tend toward NACK. I’d like to encourage people to focus on writing meaningful (and correct) documentation, not how to massage whitespace and formatting to make the linter happy.

TBH there’s too much of these PRs messing-around with whitespace and formatting rules lately. This strays very far from the intent of the project.

jonasschnelli commented at 12:46 pm on September 18, 2019: contributor

I also tend to NACK on this for the stated reasons. Mild code style enforcement is acceptable. Building a too strict environment leads to wasted resources though.

ch4ot1c commented at 0:42 am on September 19, 2019: contributor

I hear the overall NACK. The intent is to have consistent, readable docs - thats all.

test: Add .markdownlint.yml rules for *.md 90ca5e90b8

ch4ot1c force-pushed on Sep 24, 2019

ch4ot1c renamed this:
~~test: Add markdownlint rules~~
doc: Add markdownlint rules & apply formatting
on Sep 24, 2019

ch4ot1c force-pushed on Sep 24, 2019

doc: Format *.md according to .markdownlint.yml rules 147dfb4294

ch4ot1c force-pushed on Sep 24, 2019

laanwj commented at 10:59 am on September 30, 2019: member

Closing this, there seem to be no agreement to do this.

laanwj closed this on Sep 30, 2019

DrahtBot locked this on Dec 16, 2021

doc: Add markdownlint rules & apply formatting #16901

Conflicts