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: Add markdownlint rules & apply formatting #16901

Closed
wants to merge 2 commits into from

Conversation

ch4ot1c
Copy link
Contributor

@ch4ot1c ch4ot1c commented Sep 17, 2019

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:

markdownlint . --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.

@meshcollider
Copy link
Contributor

-0, I think this is a little unnecessary

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 18, 2019

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.

Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

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

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).

ci/retry/README.md Show resolved Hide resolved
doc/developer-notes.md Show resolved Hide resolved
@ch4ot1c
Copy link
Contributor Author

ch4ot1c commented Sep 18, 2019

=== 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..

@maflcko
Copy link
Member

maflcko commented Sep 18, 2019

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.

Copy link
Member

@promag promag left a comment

Choose a reason for hiding this comment

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

Concept ACK on improving markdown.

I also don't mind the markdown linter.

@laanwj
Copy link
Member

laanwj commented Sep 18, 2019

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
Copy link
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
Copy link
Contributor Author

ch4ot1c commented Sep 19, 2019

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

@ch4ot1c ch4ot1c changed the title test: Add markdownlint rules doc: Add markdownlint rules & apply formatting Sep 24, 2019
@laanwj
Copy link
Member

laanwj commented Sep 30, 2019

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

@laanwj laanwj closed this Sep 30, 2019
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants