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: Format docs with mdformat-myst #156

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

hukkin
Copy link
Contributor

@hukkin hukkin commented Apr 15, 2021

@chrisjsewell not sure if you want this merged, but if nothing else, this is to showcase and let you know that a MyST formatter exists 😉

In addition to auto-formatting the docs, this PR fixes an exclude in pre-commit config (benchmark folder changed to benchmarking) and fixes incorrect list indentation in contributing.md.

@codecov
Copy link

codecov bot commented Apr 15, 2021

Codecov Report

Merging #156 (c112fcc) into master (53d9b33) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #156   +/-   ##
=======================================
  Coverage   96.10%   96.10%           
=======================================
  Files          60       60           
  Lines        3232     3232           
=======================================
  Hits         3106     3106           
  Misses        126      126           
Flag Coverage Δ
pytests 96.10% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53d9b33...c112fcc. Read the comment docs.

@hukkin
Copy link
Contributor Author

hukkin commented Apr 15, 2021

RTD build will fail because it has these dependencies

  • myst-parser
  • markdown-it-py == <current-version>, that is since RTD requirements are specified as pip install .[rtd], RTD implicitly has markdown-it-py's current version as dependency

A conflict comes from the fact that myst-parser requires markdown-it-py~=0.6.2, which is incompatible with pip install ..

To fix this, I recommend making RTD not implicitly require the package itself. One solution is to remove the rtd install extra and replace it with docs/requirements.txt.

@chrisjsewell
Copy link
Member

Thanks I'll check it out soonish

But yeh I want RTD to build with the develop version of markdown-it-py.
This is a known issue, but will be less problematic once myst-parser (and jupytext) are pinning to ~1.0 where it should be a good while before a back-incompatible v2

@@ -28,8 +28,9 @@ Also [John MacFarlane](https://github.com/jgm) for his work on the CommonMark sp
- <http://talk.commonmark.org> - CommonMark forum, good place to collaborate developers' efforts.

```{toctree}
:maxdepth: 2

---
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice if this did not convert from the short-hand

Copy link
Member

Choose a reason for hiding this comment

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

but mainly looking good cheers!

Copy link
Contributor Author

@hukkin hukkin Apr 25, 2021

Choose a reason for hiding this comment

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

It would be nice if this did not convert from the short-hand

Hmm, maybe. I couldn't see any crystal clear advantage to using the short-hand, besides maybe being one line less verbose. Always formatting with the frontmatter style has a few advantages:

  • Kills useless debate in PRs about which style to use
  • Removes the chance that you'll ever need to look at a frontmatter style to short-hand style diff (or vice-versa) in a PR --> cleaner diffs, simpler reviews.
  • The syntax makes it more obvious that the content is, in fact, just plain YAML.

but mainly looking good cheers!

🥳

Copy link
Member

Choose a reason for hiding this comment

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

I couldn't see any crystal clear advantage to using the short-hand

Well its more comparable to the RST syntax (to ease migration), but mainly I fear (like the consecutive numbering discussion) it may put users off using mdformat, if it enforces a style they are not used to (a lot of current documentation uses the shorthand), similarly I would envisage it as a flag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consecutive numbering was different to me in that that style clearly is better in its own way (more readable, and resembles rendered doc more). I don't currently feel that way about the shorthand syntax.

I definitely understand the point about putting users off, but a) mdformat is an opinionated formatter, not a text passthrough function, and b) users don't pay me so I don't necessarily have to care 😄.

If someone were to contribute a --keep-directive-style flag I'd merge it.

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.

3 participants