Add yamllint check for RST code listings #2385

felixfontein · 2025-01-31T13:56:03Z

This was harder than I expected since the line number information is bonkers literal_block nodes, and the column information not there completely.

~~This currently fails in quite a few places. Once #2382 is merged this should get better though...~~

Also I currently disabled most problems - we also have to figure out the config we want to use. I guess the most pressing issues are syntax errors when it's simply not a YAML file at all ;)

Ref: #2382 (comment)

tests/checkers/rst-yamllint.py

tests/checkers/rst-yamllint.json

tests/checkers/rst-yamllint.py

webknjaz

Have you considered using sphinxawesome-codelinter? Their README has yamllint as an example: https://github.com/kai687/sphinxawesome-codelinter?tab=readme-ov-file#configure.

felixfontein · 2025-02-01T22:13:38Z

Have you considered using sphinxawesome-codelinter? Their README has yamllint as an example: https://github.com/kai687/sphinxawesome-codelinter?tab=readme-ov-file#configure.

I haven't because I wasn't aware of it. Looking at it, I also think it would perform worse than the code in this PR:

It does not adjust line and column offsets, you have to do your own calculations.
It does not warn about codeblocks with missing language, unsupported languages, or use of ::.

felixfontein · 2025-02-12T18:47:54Z

Currently contains #2408. Will rebase once that's merged.

oraNod

@felixfontein Looks really good to me. I had a couple minor suggestions on warnings. Other than that I think it's great. Maybe we could add some docstrings to classes to improve things further.

Thanks very much for working on this!

tests/checkers/rst-yamllint.py

webknjaz · 2025-02-28T13:09:40Z

Have you considered using sphinxawesome-codelinter? Their README has yamllint as an example: kai687/sphinxawesome-codelinter#configure.

I haven't because I wasn't aware of it. Looking at it, I also think it would perform worse than the code in this PR:
1. It does not adjust line and column offsets, you have to do your own calculations.

2. It does not warn about codeblocks with missing language, unsupported languages, or use of `::`.

Might be worth contributing this code there then?

felixfontein · 2025-02-28T21:00:33Z

tests/checkers/rst-yamllint.py

+                        "path": self.__path,
+                        "line": node.line or "unknown",
+                        "col": 0,
+                        "message": f"Warning: found unknown literal block! Check for double colons '::'. If that is not the cause, please report this warning. It might indicate a bug in the checker or an unsupported Sphinx directive. Node: {node!r}; attributes: {node.attributes}; content: {node.rawsource!r}",


Suggested change

"message": f"Warning: found unknown literal block! Check for double colons '::'. If that is not the cause, please report this warning. It might indicate a bug in the checker or an unsupported Sphinx directive. Node: {node!r}; attributes: {node.attributes}; content: {node.rawsource!r}",

"message": f"Warning: found unknown literal block! Check for double colons '::' and convert them to code blocks. If that is not the cause, please report this warning. It might indicate a bug in the checker or an unsupported Sphinx directive. Node: {node!r}; attributes: {node.attributes}; content: {node.rawsource!r}",

@oraNod ^ is that better?

Ah, right. It's implicit code blocks (I guess that's the right way to say it). I forgot you could just have naked :: double colons. Anyway this looks like a good improvement to me.

felixfontein · 2025-02-28T21:02:12Z

Have you considered using sphinxawesome-codelinter? Their README has yamllint as an example: kai687/sphinxawesome-codelinter#configure.

I haven't because I wasn't aware of it. Looking at it, I also think it would perform worse than the code in this PR:
1. It does not adjust line and column offsets, you have to do your own calculations.

2. It does not warn about codeblocks with missing language, unsupported languages, or use of `::`.
Might be worth contributing this code there then?

Definitely. I don't think I have time for that anytime soon, though...

felixfontein · 2025-03-01T14:58:04Z

I've also made sure that regular YAML files in the repository pass yamllint .. (I think if we have a yamllint config, it should apply to all YAML files.) I'm not sure though where to add it into CI. Should it be a separate GitHub workflow? A separate Nox session (part of lint)? Should it be a separate checker (these should usually only work on files in docs/, not in other parts of the repo)?

oraNod · 2025-03-03T16:10:08Z

I've also made sure that regular YAML files in the repository pass yamllint .. (I think if we have a yamllint config, it should apply to all YAML files.) I'm not sure though where to add it into CI. Should it be a separate GitHub workflow? A separate Nox session (part of lint)? Should it be a separate checker (these should usually only work on files in docs/, not in other parts of the repo)?

I think the current scope is good, just the docs/ folder. I also think it is fine to leave as part of the checkers session instead of a new nox session. I like that the checkers session is a light enough wrapper. Maybe we don't want to have too much overlapping logic.

Maybe the only thing really missing is an entry in the README to explain how folks can use the checkers test to verify their changes as part of a PR? https://github.com/ansible/ansible-documentation/blob/devel/README.md#running-automated-tests

webknjaz · 2025-03-06T18:45:40Z

I've also made sure that regular YAML files in the repository pass yamllint .. (I think if we have a yamllint config, it should apply to all YAML files.) I'm not sure though where to add it into CI. Should it be a separate GitHub workflow? A separate Nox session (part of lint)? Should it be a separate checker (these should usually only work on files in docs/, not in other parts of the repo)?

In a typical Python project, I'd have it as a part of the pre-commit config. Here, it might make sense to add it to lint. And the config would have to be in the repo root. Example: https://github.com/ansible/awx-plugins/blob/e5f7468/.yamllint.

webknjaz · 2025-03-06T18:46:34Z

.github/workflows/build-package-docs.yaml

@@ -1,7 +1,7 @@
 ---

 name: Ansible package docs build
-on:
+"on":


FWIW, dropping a config like https://github.com/ansible/awx-plugins/blob/e5f7468/.yamllint into the repo would allow these.

Generally enabling on just for a handful of special YAML files sounds like a bad idea.

webknjaz · 2025-03-06T18:47:27Z

Have you considered using sphinxawesome-codelinter? Their README has yamllint as an example: kai687/sphinxawesome-codelinter#configure.

I haven't because I wasn't aware of it. Looking at it, I also think it would perform worse than the code in this PR:
1. It does not adjust line and column offsets, you have to do your own calculations.

2. It does not warn about codeblocks with missing language, unsupported languages, or use of `::`.
Might be worth contributing this code there then?
Definitely. I don't think I have time for that anytime soon, though...

Ack. I just realized I'd love to use this elsewhere myself.

felixfontein · 2025-03-06T18:54:18Z

Here, it might make sense to add it to lint.

Either it needs a new session, with new lock files, or an existing session needs to be extended (with new requirements). (Neither is anything I want to do in this PR.)

And the config would have to be in the repo root.

It's already there.

Co-authored-by: Don Naro <[email protected]>

…ve line lengths.

webknjaz · 2025-03-06T19:05:25Z

And the config would have to be in the repo root.

It's already there.

Are you talking about this repo? I don't see it.

felixfontein · 2025-03-06T19:13:48Z

https://github.com/ansible/ansible-documentation/pull/2385/files

This uses a third-party Sphinx extension now [[1]]. In the future, though, we should explore using the solution from ansible-documentation [[2]]. [1]: https://github.com/kai687/sphinxawesome-codelinter [2]: ansible/ansible-documentation#2385

webknjaz · 2025-03-06T19:31:33Z

#2385 (files)

Oh, I thought you were talking about something pre-existing :) Thanks for pointing this out!

oraNod

Maybe some of the more experienced Pythonistas around here will have more feedback, but this looks really solid to me.

Thanks for doing this @felixfontein It's a great improvement to the quality of the docs.

felixfontein temporarily deployed to github-bot January 31, 2025 13:56 — with GitHub Actions Inactive