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

Enable rstcheck in CI and lint code blocks #498

Merged
merged 8 commits into from
Mar 18, 2024
Merged

Enable rstcheck in CI and lint code blocks #498

merged 8 commits into from
Mar 18, 2024

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Feb 11, 2024

This PR follows up on #493 by following GitHub's advice of running the spec through rstcheck. We were already passing it, but in the process of adding it I think it makes sense to properly lint our code blocks as JSON data (which is possible in many cases without losing the structure of them).

Also this PR:

  • unifies the indent of all code blocks to start after the 3 chars indicated with the RST ".. ".
  • fixes one occurrence of a non-standard field in a code block ("response_message" for the meta example)
  • fixes indentation of one implementation note so that it refers to its proper field
  • switches the partial data code blocks to JSONL format

Unfortunately its a bit tricky to see how this will render in the future in GitHub, so perhaps we need to hold off a little while for the official response.

@rartino
Copy link
Contributor

rartino commented Mar 18, 2024

@ml-evs This is marked as a draft, but is it not good to go now? The GitHub pre-commit checks pass and seem to include rstcheck.

(That said, in some future cleanup I would like to just float if it is possible to move these checks into 'make audit' instead, and then only have a single 'make audit' in the pre-commit hooks. That makes it a bit more straightforward to keep the pre-commit hooks aligned with the checks it is easy to do oneself when working on a clone, and it would also avoid us becoming unnecessarily dependent on git-specific features.)

@ml-evs ml-evs marked this pull request as ready for review March 18, 2024 09:34
@ml-evs ml-evs requested review from rartino and merkys March 18, 2024 13:10
@ml-evs
Copy link
Member Author

ml-evs commented Mar 18, 2024

In that case, feel free to review and we can merge here. Otherwise also happy to use make audit straight away. There might be some conflicts here with #499 -- perhaps this should be merged first as it introduces additional checks.

rartino
rartino previously approved these changes Mar 18, 2024
@rartino rartino mentioned this pull request Mar 18, 2024
vaitkus
vaitkus previously approved these changes Mar 18, 2024
Co-authored-by: Antanas Vaitkus <antanas.vaitkus90@gmail.com>
@ml-evs ml-evs dismissed stale reviews from vaitkus and rartino via a8742b7 March 18, 2024 18:19
@ml-evs ml-evs requested review from rartino and vaitkus March 18, 2024 18:20
@ml-evs ml-evs merged commit dfb0ca5 into develop Mar 18, 2024
5 checks passed
@ml-evs ml-evs deleted the ml-evs/rstcheck branch March 18, 2024 18:38
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.

4 participants