Suggestion to treat warnings as errors in sphinx-build

[sfmig]

Description

What is this PR

• Bug fix
• Addition of a new feature
• Other -- a suggestion

Why is this PR needed?
On issue #252, @lochhh pointed out that sphinx-build returns a successful exit code (0) even if it encountered errors.

This can be quite annoying: in practice it means we need to remember to manually check the logs of the docs building action (or locally), to confirm that there were no errors, even if the action shows a pass ✅ .

For example, in the case @lochhh caught, the docstrings were not rendered correctly in the API reference, but the action was passing.

The simplest workaround seems to add the flags -W --keep-going so that sphinx-build runs to completion, but treating warnings as errors and returning a non-zero exit code if there are errors or warnings.

What does this PR do?

This PR adds the flags -W --keep-going to our Makefile. Open to discussion though!

Question:
Should we add these flags to the neuroinformatics action too? It may be useful for everyone. If we agree we go for this, I am happy to do it.

References

• sphinx-build exit code is 0 with ERRORS sphinx-doc/sphinx#10397
• Unexpected indentations in movement.validators.files.ValidVIATracksCSV docs #252, and also the PRs mentioned in that issue

How has this PR been tested?

I manually checked the exit code from sphinx-build with and without the proposed flags, in two situations:

• Case 1: sphinx-build returns a warning only
  To reproduce:

  1. I edited the docstring of a user visible function (e.g. movement.io.load_bboxes-->from_numpy) to indent something under the parameter section.
  2. I run make clean html from the docs directory
  3. That produces a WARNING only:

   /Users/sofia/swc/project_movement_dataloader/movement/movement/io/load_bboxes.py:docstring of movement.io.load_bboxes.from_numpy:10: WARNING: Field list ends without a blank line; unexpected unindent.

• Case 2: sphinx-build returns an error
  To reproduce:

  1. I edited the docstring of a user visible function (e.g. movement.io.load_bboxes-->from_numpy) to indent something under the main summary line, but before the Parameters section.

   """Create a ``movement`` bounding boxes dataset from NumPy arrays.
   
     Bla
     - bla   
         bla
   
     Parameters
     ----------
     ...
   """

  3. I run make clean html from the docs directory
  4. That produces an ERROR:

   /Users/sofia/swc/project_movement_dataloader/movement/movement/io/load_bboxes.py:docstring of movement.io.load_bboxes.from_numpy:6: ERROR: Unexpected indentation.

In both cases, if we run make clean html

• without any flags to sphinx-build in the Makefile ---> I get exit code 0
• with the flags -W --keep-going added to sphinx-build in the Makefile---> I get exit code 2

Is this a breaking change?

No.

Does this PR require an update to the documentation?

I added a comment to the workflow file to clarify

Checklist:

• The code has been tested locally
• [ n/a ] Tests have been added to cover all new functionality
• The documentation has been updated to reflect any changes
• The code has been formatted with pre-commit

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.76%. Comparing base (d3fac95) to head (8aa8feb).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #256   +/-   ##
=======================================
  Coverage   99.76%   99.76%           
=======================================
  Files          14       14           
  Lines         866      866           
=======================================
  Hits          864      864           
  Misses          2        2           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[niksirbi]

I like this @sfmig, that particular loophole has created a few problems for us already and it'd be nice to close.

I'd ask you to also update the sphinx-build commands in "Building the documentation locally" section of the contributing guide (under the "All platforms" tabs). The -W --keep-going should also be included there in case people cannot (or choose to not) use the make command.

I'd also favour updating the corresponding NIU action.

[sfmig]

One question to our Make expert @lochhh:

Should we add the flags -W --keep-going to SPHINXOPTS, rather than appending them to the sphinx-build command? WDYT?

[lochhh]

One question to our Make expert @lochhh:

Should we add the flags -W --keep-going to SPHINXOPTS, rather than appending them to the sphinx-build command? WDYT?

😄 no Make expert, but I think we should use SPHINXOPTS, because it's what it is for, and so that in both make and sphinx-build commands in the guide, we consistently have the flags (we can also explain what they do). Alternatively, we can keep the guide as is (throwing only warnings), whilst mentioning the flags (what they do and that they're used in CI) and only enable these in CI doc build job

[niksirbi]

One question to our Make expert @lochhh:

Should we add the flags -W --keep-going to SPHINXOPTS, rather than appending them to the sphinx-build command? WDYT?

If SPHINXOPTS is meant to override the default values of sphinx-build flags, and therefore implicitly applies -W --keep-going when a user runs sphinx-build locally, yes.

[niksirbi]

Alternatively, we can keep the guide as is (throwing only warnings), whilst mentioning the flags (what they do and that they're used in CI) and only enable these in CI doc build job

I'm not a fan of this, as it would results in differences between local build vs CI, which could be confusing during debugging. I think we should try to stick to the rule that local vs CI checks should be as similar as possible.

[sfmig]

and therefore implicitly applies -W --keep-going when a user runs sphinx-build locally, yes.

Just to clarify, adding them to SPHINXOPTS would only apply the flags to the sphinx-build command when a user builds the docs with make html

[niksirbi]

Just to clarify, adding them to SPHINXOPTS would only apply the flags to the sphinx-build command when a user builds the docs with make html

Thanks for the clarifications. Then my initial suggestion for also adding these flags to the sphinx-build commands in the contributing guide still stands.

[sfmig]

Ok just to recap:

• the suggested flags have been added to contributing guide
• in the Makefile, the flags have been added via the SPHINXOPTS variable (as that seems to be its intended use; before I just appended the flags to the last sphinx-build command in the makefile)

This way both documented ways of building the docs locally (via make, or via explicit sphinx-build commands) match.

Thanks both!

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud