search

canonical / sphinx-docs-starter-pack

A documentation starter-pack
https://canonical-starter-pack.readthedocs-hosted.com/
Other
13 stars 35 forks source link

Doc checks should throw non-zero exit code when catching invalid format error #175

Closed agileshaw closed 2 months ago

agileshaw commented 7 months ago

As seen in this workflow result, there is an error with code block which has been detected in spell check. However, the check ended successfully (in both local run and workflow), which gives developer a false idea of the documentation correctness.

I think the check should throw non-zero exit code in this situation.

ru-fu commented 7 months ago

Well, that's an interesting case ...

The failure here is actually in the documentation build, NOT in the spelling check. The spelling check works in the way that it first builds the output and then runs the check on the output, so what you are seeing is the build failure, not the check failure. And I don't agree that the spelling check should fail in this case - it should run even if the build isn't successful (this might save you a few iterations of fixing both build and spelling check ;) ).

You should, however, get a failure on the PR for the actual documentation build. Not sure if you have the RTD build set up so that it builds on PRs and displays its result in the GitHub checks?

A question could be if we need to add an (optional?) check for a successful build into the doc workflow.

akcano commented 6 months ago

A broken doc build should definitely fail the PR. Warnings are fine sometimes, but errors usually indicate something is seriously askew. I can overtake it from here if you're OK with that.

ru-fu commented 6 months ago

A broken doc build should definitely fail the PR. Warnings are fine sometimes, but errors usually indicate something is seriously askew. I can overtake it from here if you're OK with that.

Sure, fine with me! But as I said, this should fail somewhere outside of the spelling check imo.