-
Notifications
You must be signed in to change notification settings - Fork 160
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
tools: add editor and lint configuration #4328
base: master
Are you sure you want to change the base?
Conversation
Add a standardized vscode configuration that should lead to a more consistent code base. Additionally, switch to markdownlint-cli that is well supported in vs code and other editors. The rules are kept the same, but we extend linting to our documentation to. To satisfy the linter, we switch to myst parser for sphinx which allows us to drop the hacky html tags in the markdown files. Myst is as powerful as RST with a more sane syntax.
881fb7d
to
2f31150
Compare
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'm ok with adding the vscode configuration, but I don't think the "lead to a more consistent code base" can really be an expectation. I understood that this can help to get people started quickly, but this will not be mandatory.
Using markdownlint-cli2 instead of the ruby thingy seems great!
Using Myst could have been a sensible choice, but my vote is to keep using restructuredText for any new documentation exclusively. Only a few old pages in the documentation use markdown and in my opinion we can just leave them alone (e.g. exclude them from the linter if that helps) and/or eventually convert it to restructuredText with a conversion tool. New documentation should only be added in restructuredText.
Reviewed 9 of 19 files at r1, all commit messages.
Reviewable status: 9 of 19 files reviewed, 1 unresolved discussion (waiting on @oncilla)
doc/requirements.txt
line 5 at r1 (raw file):
# To update, run: # # bazel run //doc:requirements.update
Can the documentation be built with bazel, and if so, how? Otherwise, I don't see the point of involving bazel here.
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.
can really be an expectation
Fair enough. I will rephrase.
Using Myst could have been a sensible choice, but my vote is to keep using restructuredText for any new documentation exclusively.
myst has a far more sensible syntax than rst, and it has the same set of capabilities (worst case you can always fall back in rst, if necessary. But I have never had to so far in the docs that I have written). Due to this, it is also a lot more approachable than rst, which makes it easier for people to contribute. Markdown is the de-facto standard for documentation in most ecosystems. This also reflects in the quality of tools that you have at your disposal: editor support, lint support, etc. They are far better in the markdown landscape than the rst landscape.
If your concern is a split of documentation syntaxes, I think we can easily migrate the docs to be myst only: https://github.com/executablebooks/rst-to-myst
Reviewable status: 9 of 19 files reviewed, 1 unresolved discussion (waiting on @matzf)
doc/requirements.txt
line 5 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Can the documentation be built with bazel, and if so, how? Otherwise, I don't see the point of involving bazel here.
This ensures that this file is deterministically created, no matter what the python installation on the developers machine looks like.
We can easily serve the documentation via bazel. If you want to, I can extend it to do so.
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.
My concern is churn. What I want to avoid is to zig-zag between different tools on a whim (e.g. as an aside of some unrelated change, like this one). I see the upsides of Myst, it does look quite cool!
But anyway, switching away from RST is a big enough decision to warrant a proper, separate proposal and a decision (that we should then stick to).
Reviewable status: 9 of 19 files reviewed, all discussions resolved
doc/requirements.txt
line 5 at r1 (raw file):
Ok, fair enough (python version used here is the one defined in WORKSPACE). It's a small diff from what we already have, so ok, I guess.
But I do find it it rather annoying to cover the already fragmented python packaging tooling under another layer of non-standard and barely documented bazel tooling. 🤷♂️
Can you please adapt .readthedocs.yaml
to use python 3.10, too?
We can easily serve the documentation via bazel. If you want to, I can extend it to do so.
Yes, bazel can eat the world. 😄 But no, that doesn't sound good. This would mean that we'd have to install bazel on readthedocs builds.
Use Node.js-based github.com/DavidAnson/markdownlint-cli2, replacing the ruby-based github.com/markdownlint/markdownlint. This tool has a cleaner configuration file and that is well supported in vs code and other editors. Adapted from scionproto#4328. Co-authored-by: Dominik Roos <roos@anapaya.net>
Use Node.js-based github.com/DavidAnson/markdownlint-cli2, replacing the ruby-based github.com/markdownlint/markdownlint. This tool has a cleaner configuration file and that is well supported in vs code and other editors. Adapted from scionproto#4328. Co-authored-by: Dominik Roos <roos@anapaya.net>
Use Node.js-based github.com/DavidAnson/markdownlint-cli2, replacing the ruby-based github.com/markdownlint/markdownlint. This tool has a cleaner configuration file and that is well supported in vs code and other editors. Adapted from scionproto#4328. Co-authored-by: Dominik Roos <roos@anapaya.net>
Use Node.js-based github.com/DavidAnson/markdownlint-cli2, replacing the ruby-based github.com/markdownlint/markdownlint. This tool has a cleaner configuration file and that is well supported in vs code and other editors. Adapted from scionproto#4328. Co-authored-by: Dominik Roos <roos@anapaya.net>
Use Node.js-based github.com/DavidAnson/markdownlint-cli2, replacing the ruby-based github.com/markdownlint/markdownlint. This tool has a cleaner configuration file and that is well supported in vs code and other editors. Adapted from scionproto#4328. Co-authored-by: Dominik Roos <roos@anapaya.net>
- use markdownlint-cli2 instead of ruby markdownlint Use Node.js-based github.com/DavidAnson/markdownlint-cli2, replacing the ruby-based github.com/markdownlint/markdownlint. This tool has a cleaner configuration file and that is well supported in vs code and other editors. Adapted from #4328. - add sphinx-lint, and manage the doc requirements via bazel/rules_python. Fix a large number of legitimage linter warnings; most cases were accidentally applying the "default role" (single backticks instead of double-backticks), or the occasional missing underscore after external links. - add bazel run targets to run sphinx-build and sphinx-autobuild via python deps managed by bazel. This makes it easy to locally build the documentation for devs that already have bazel set up (while hopefully keeping things still relatively straight-forward). --------- Co-authored-by: Dominik Roos <roos@anapaya.net>
Add a standardized vscode configuration that should lead to a more consistent code base. Additionally, switch to markdownlint-cli that is well supported in vs code and other editors.
The rules are kept the same, but we extend linting to our documentation to. To satisfy the linter, we switch to myst parser for sphinx which allows us to drop the hacky html tags in the markdown files. Myst is as powerful as RST with a more sane syntax.
This change is