-
Couldn't load subscription status.
- Fork 85
📚 DOCS: Format docs with mdformat-myst #156
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
Conversation
Codecov Report
@@ Coverage Diff @@ ## master #156 +/- ## ======================================= Coverage 96.10% 96.10% ======================================= Files 60 60 Lines 3232 3232 ======================================= Hits 3106 3106 Misses 126 126
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
RTD build will fail because it has these dependencies
myst-parsermarkdown-it-py == <current-version>, that is since RTD requirements are specified aspip install .[rtd], RTD implicitly hasmarkdown-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.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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!
🥳
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
@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 (
benchmarkfolder changed tobenchmarking) and fixes incorrect list indentation incontributing.md.