Emit "source-read" events for files read via the include directive
#11510
Conversation
Our unit test is now passing.
Why doesn't the project apply these changes automatically on the main branch?
These tests needed to change with the introduction of the derived class `sphinx.parsers.RSTStateMachine`.
Noting that docutils 0.18 introduced the marker.
This reverts commit 4c727ce.
The subclassing will not work because docutils doesn't use Sphinx's rst parser for processing the included rst text. This is a shame, and strictly speaking an issue in Sphinx/docutils. So we go back to monkey patching. It works, the change is now better contained inside Sphinx Include directive. Added a unit test to verify that includes of includes are correctly handled.
…into issue-10678
|
@picnixz Docutils HEAD is busted - doesn't work with Sphinx. I tried the 7.0.x branch and it fails the same way. That is the only failure on this PR. Do we wait for this to get fixed? |
|
Ah yes, that docutils HEAD failing test. I've had one even before (#11486 (comment)) but I don't know the reason. At first, I thought it was because it's getting the dev version of docutils (and then you get docutils 0.21dev or something like that) but sometimes it works and sometimes not. Just ignore it for now. @AA-Turner Do you have any idea why the test fails ? (and why it fails on other PRs that do not touch at all the docutils layout?) EDIT: One possibility is that there is some race condition and outputs are wiped out before being tested or something like that. |
|
571becb should have fixed this. I’m travelling today, will look when I return. A |
These are not part of my change but mysteriously now cause failure in CI.
created Unreleased section and moved my change notes to it.
|
All righty then, by targeting the new 7.1.x branch we have docutils passing. Everything else is also passing. |
|
@AA-Turner @picnixz I believe this PR is ready. |
# Conflicts: # CHANGES
There was a problem hiding this comment.
Just wondering but do you think it's worth to actually have this behaviour disabled by default? because if there are a lot of included files that are actually not processed by the source-read event, then the overhead might not be negligible for very large projects (I don't know if large projects use include directives a lot or not). Maybe it's good to avoid slowing down builds (you essentially perform "join/splits" twice which probably increases your build time by 10-20% if you have a lot of files). If my question is actually a non-question, LGTM.
AA-Turner
changed the title
include directive
Co-authored-by: picnixz <[email protected]>
Yes, I think this is worth while. We would then make it a configuration variable to have this functionality enabled. You will need to suggest what it should be named. But how would that effect the tests? I would need to see how we can enable the config variable for each test. Shouldn't be hard. The patch is also done "just-in-time" so it is easy to turn on and off. |
# Conflicts: # CHANGES
|
@halldorfannar is there anything outstanding here from your perspective? A |
# Conflicts: # sphinx/directives/other.py
|
Thanks! A |
|
In my case this fix triggers an infinite recursion, because the included file contains a custom directive that uses Worked around by also using Maybe worth mentioning, since other directives doing similar things may break too. (like maybe longturn/freeciv21#2023) |
|
@johannesloibl please may you open a new issue for further discussion? A |
Subject: Extend Sphinx to emit "source-read" events for RST files coming in via
includedirective.Feature or Bugfix
Purpose
Detail
Changes are well documented - please consult the code. Added a thorough unit test to expose the issue and fix it.
Relates