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

tools: add editor and lint configuration #4328

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

Conversation

oncilla
Copy link
Contributor

@oncilla oncilla commented Mar 14, 2023

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 Reviewable

@oncilla oncilla requested a review from matzf March 14, 2023 17:43
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.
Copy link
Member

@matzf matzf left a 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.

Copy link
Contributor Author

@oncilla oncilla left a 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.

Copy link
Member

@matzf matzf left a 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.

matzf added a commit to matzf/scion that referenced this pull request Apr 8, 2024
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>
matzf added a commit to matzf/scion that referenced this pull request Apr 8, 2024
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>
matzf added a commit to matzf/scion that referenced this pull request Apr 11, 2024
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>
matzf added a commit to matzf/scion that referenced this pull request Apr 19, 2024
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>
matzf added a commit to matzf/scion that referenced this pull request Apr 19, 2024
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>
matzf added a commit that referenced this pull request Apr 19, 2024
- 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>
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.

None yet

2 participants