Skip to content

Conversation

@tsbinns
Copy link
Contributor

@tsbinns tsbinns commented Oct 29, 2025

What does this implement/fix?

Noticed some of the versionadded Sphinx directives were not rendering properly in the docs. Checked also for other directives while I was at it, but I only saw this with cases of versionadded.

@larsoner
Copy link
Member

FYI if you want to go the extra mile... we could hook into the Sphinx event system and make sure any .. versionadded are preceded by an empty line (at least following a strip). We could hook into source-read:

https://www.sphinx-doc.org/en/master/extdev/event_callbacks.html#event-source-read

and look at contents. You could try doing it on main and code it to catch these errors and emit Sphinx warnings (which we treat as errors in doc builds).

We could add the same for versionchanged for example.

And we probably never want the regular expressionr" +\.\.[a-zA-Z]+" to show up in contents, that would have caught ..versionadded

Feel free to give it a shot here, otherwise just create an issue from this comment with a suitable title and someone can work on it at some point when they're motivated

@tsbinns
Copy link
Contributor Author

tsbinns commented Oct 29, 2025

Happy to give this a go. Would the appropriate way be to add a new script to doc/sphinxext that connects on builder-inited?

@larsoner
Copy link
Member

I would connect it to source-read not builder-inited. But yes doc/sphinxext would make sense. We hook into some other stuff as well already if you look at doc/conf.py

@tsbinns
Copy link
Contributor Author

tsbinns commented Oct 29, 2025

Ah sorry, didn't realise source-read was its own event.

@tsbinns
Copy link
Contributor Author

tsbinns commented Oct 30, 2025

Having issues building the docs locally, so reverting the earlier fixes and want to see if they get picked up by the new script here.

@tsbinns
Copy link
Contributor Author

tsbinns commented Oct 30, 2025

Okay, so the script can detect cases of bad formatting in e.g., examples and tutorials (and is picking up an instance I wasn't aware of before), but the content from source-read doesn't contain the docstrings, so nothing is being picked up there.

@tsbinns
Copy link
Contributor Author

tsbinns commented Oct 30, 2025

@larsoner source-read gives access to the content in autoexamples and autotutorials which we can check for bad formatting, but the rst files in generated don't contain the explicit docstrings (just autoclass/autofunction).

Is there a way to access the docstrings at the read phase, or do we have to rely on e.g., checking the html files once they've been written?

@larsoner
Copy link
Member

How about one of

https://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html#docstring-preprocessing

@tsbinns
Copy link
Contributor Author

tsbinns commented Oct 30, 2025

Sorry for the delay, had to fight some new sphinx-build - Windows compatibility issues I did not have before.

How about one of
https://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html#docstring-preprocessing

That was exactly it, thanks @larsoner!

Have updated the script so it'll also check docstrings. Couple things to note which I will look into later:

  • The code could probably be tidied up.
  • There are some cases where warnings are repeated, but I'm not sure why.
  • I also have some concerns about efficiency, since in this state it's checking the contents of all sources line-by-line, but I haven't tested to see how much this is slowing the docs building down.

@tsbinns
Copy link
Contributor Author

tsbinns commented Oct 30, 2025

It's also picking up this case in the examples where the references aren't properly separated from the code.

fig, ax = plt.subplots()
ax.plot(freq, psd, label="raw")
ax.plot(freq, psd_proc, label="processed")
ax.text(0.2, 0.7, "cortical", transform=ax.transAxes)
ax.text(0.8, 0.25, "subcortical", transform=ax.transAxes)
ax.set(ylabel="EEG Power spectral density", xlabel="Frequency (Hz)")
ax.legend()
# References
# ^^^^^^^^^^
#
# .. footbibliography::

image

This gives the following warning: WARNING: File 'auto_examples/preprocessing/css' is missing a blank line before the directive '.. footbibliography::'. So the message doesn't work great since there is a blank line, it's just not processed as such.
Is it good enough that the warning is pointing to something being wrong here, or should there be a different warning message for this special case?

@larsoner
Copy link
Member

Is it good enough that the warning is pointing to something being wrong here, or should there be a different warning message for this special case?

I think it's okay if the warning isn't perfect / correct, as long as there is something to actually fix (and there is here!). What would be good to avoid is false alarms

@larsoner
Copy link
Member

... and if there are some false alarms, we can build some mechanism / allowlist of exceptions. But maybe it wont be needed yet, we'll see!

@tsbinns
Copy link
Contributor Author

tsbinns commented Oct 31, 2025

What would be good to avoid is false alarms

Everything that gets picked up now is a genuine case of the directive not rendering properly.

and if there are some false alarms, we can build some mechanism / allowlist of exceptions.

The exceptions I found so far where a blank line is not necessary are:

  • it's right after a header marked by a line of all ----, ====, or ^^^^ (e.g., in examples, tutorials, docstring notes)
  • the previous line is a directive or is content for a directive

dir_pattern = r"\.\. [a-zA-Z]+::"
head_pattern = r"^[-|=|\^]+$"
directive = re.search(dir_pattern, line)
if directive is not None:
line_prev = source[idx - 1].strip()
if (
line_prev != "" # not an empty line
and not re.search(head_pattern, line_prev) # not after a header
and not re.search(dir_pattern, line_prev) # not after directive
):
# check if previous line is part of another directive
bad = True
for line_prev in reversed(source[: idx - 1]):
line_prev = line_prev.strip()
if line_prev == "" or re.search(head_pattern, line_prev):
# if a blank line or header, is not part of another directive
break
if re.search(dir_pattern, line_prev):
bad = False # is part of another directive
break
# or keep going until we reach the first line (so must be bad)

@larsoner
Copy link
Member

The warnings/errors look great!

https://app.circleci.com/pipelines/github/mne-tools/mne-python/29129/workflows/aa0189b1-a1ff-4eb6-bd38-b706ce646e37/jobs/76716

I think you can fix them and we can merge

@tsbinns
Copy link
Contributor Author

tsbinns commented Nov 1, 2025

Just pushed a commit that:

  • expanded the comments.
  • added a check that will catch a change in the number of args from the Sphinx events if that ever happens in a future version.
  • added an initial check that a directive occurs anywhere in the whole file/docstring before searching line-by-line.

Warnings are still the same (https://app.circleci.com/pipelines/github/mne-tools/mne-python/29136/workflows/b74c3e86-2455-40c9-ba7b-384d5796bb65/jobs/76730/parallel-runs/0/steps/0-143), will fix these cases now.

@tsbinns
Copy link
Contributor Author

tsbinns commented Nov 1, 2025

Warnings are fixed now!

@drammock drammock enabled auto-merge (squash) November 1, 2025 17:33
@drammock drammock merged commit 7cfcc6b into mne-tools:main Nov 1, 2025
32 checks passed
@tsbinns tsbinns deleted the fix_directives branch November 3, 2025 12:31
larsoner added a commit to BeiGeJin/mne-python that referenced this pull request Nov 5, 2025
* upstream/main: (230 commits)
  FIX: Fix ICA.apply when fitted including marked bad channels (mne-tools#13478)
  FIX: Correctly set the calibration factor in Nihon Kohden reader (mne-tools#13468)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#13479)
  MAINT: Update code credit (mne-tools#13477)
  Fix `versionadded` directive formatting (mne-tools#13471)
  typo in mailmap (mne-tools#13475)
  FIX: Fix _plot_topomap channel names plotting when using a mask (mne-tools#13470)
  FIX: Handle an Eyelink File with blank lines injected throughout file (mne-tools#13469)
  ENH: adds annotation filtering to raw and ica source figures (mne-tools#13460)
  MAINT: Restore VTK nightly wheel on Linux and bump changelog checker (mne-tools#13436)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#13465)
  FIX: Fix add_reference_channels for passing two channels names (mne-tools#13466)
  ENH: Add on_missing for combine_channels (mne-tools#13463)
  Bump the actions group with 2 updates (mne-tools#13464)
  Move development dependencies into a dependency group (no more extra) (mne-tools#13452)
  ENH: add on_missing for rename_channels (mne-tools#13456)
  add advisory board to website (mne-tools#13462)
  ENH: Support Nihon Kohden EEG-1200A V01.00 (mne-tools#13448)
  MAINT: Update dependency specifiers (mne-tools#13459)
  ENH: Add encoding parameter to Nihon Kohden reader (mne-tools#13458)
  ...
larsoner added a commit to szz-dvl/mne-python that referenced this pull request Nov 5, 2025
* upstream/main: (85 commits)
  FIX: Fix ICA.apply when fitted including marked bad channels (mne-tools#13478)
  FIX: Correctly set the calibration factor in Nihon Kohden reader (mne-tools#13468)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#13479)
  MAINT: Update code credit (mne-tools#13477)
  Fix `versionadded` directive formatting (mne-tools#13471)
  typo in mailmap (mne-tools#13475)
  FIX: Fix _plot_topomap channel names plotting when using a mask (mne-tools#13470)
  FIX: Handle an Eyelink File with blank lines injected throughout file (mne-tools#13469)
  ENH: adds annotation filtering to raw and ica source figures (mne-tools#13460)
  MAINT: Restore VTK nightly wheel on Linux and bump changelog checker (mne-tools#13436)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#13465)
  FIX: Fix add_reference_channels for passing two channels names (mne-tools#13466)
  ENH: Add on_missing for combine_channels (mne-tools#13463)
  Bump the actions group with 2 updates (mne-tools#13464)
  Move development dependencies into a dependency group (no more extra) (mne-tools#13452)
  ENH: add on_missing for rename_channels (mne-tools#13456)
  add advisory board to website (mne-tools#13462)
  ENH: Support Nihon Kohden EEG-1200A V01.00 (mne-tools#13448)
  MAINT: Update dependency specifiers (mne-tools#13459)
  ENH: Add encoding parameter to Nihon Kohden reader (mne-tools#13458)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants