Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an 'include-read' event #11657

Merged
merged 9 commits into from Aug 30, 2023
Merged

Conversation

AA-Turner
Copy link
Member

closes #11648, #11643, #11620

cc: @mgeier @lmoureaux

The easy option would be to set the docname to self.env.docname (the current document being read), but this might throw off extensions that expect to only have "source-read" called exactly once per document.

Please test / let me know thoughts!

A

@AA-Turner
Copy link
Member Author

I'm leaning towards the generated docname approach -- would appreciate your opinion @mgeier, as this solves the 'docname can be None' issue, but introduces docnames that don't work with doc2path().

A

@mgeier
Copy link
Contributor

mgeier commented Aug 30, 2023

I think inventing a string (like 'include-from-mysource') is much worse than returning None.

If a string is returned, I would expect that string to be the document name.

I'm fine with returning None, but the documentation should be updated, as I mentioned in #11648.

@AA-Turner
Copy link
Member Author

A good point -- I've also re-visited the original issue (#10678) and have thusly changed my mind -- this PR now introduces a new 'include-read' event, which no longer abuses the 'source-read' event for non-documents.

I assume this would work for your purposes?

A

@AA-Turner AA-Turner changed the title Fix docname is None Add an 'include-read' event Aug 30, 2023
@mgeier
Copy link
Contributor

mgeier commented Aug 30, 2023

That sounds much cleaner!

I assume this would work for your purposes?

Yes, sure, an additional event will not break anything if I don't use it, right?

My extension sphinx-last-updated-by-git doesn't need an event for the include directive, it only needs to check the env.dependencies of every source file, which should contain the included files.

@mgeier
Copy link
Contributor

mgeier commented Aug 30, 2023

I don't know if anyone needs this, but you could pass the parent document name to the event handler?

@AA-Turner
Copy link
Member Author

Good shout!

- Add to core events
- Include the parent docname
- Relative path
@AA-Turner AA-Turner merged commit ff18318 into sphinx-doc:master Aug 30, 2023
24 of 27 checks passed
@AA-Turner AA-Turner deleted the include-docname-None branch August 30, 2023 21:21
@lucyleeow
Copy link

Thanks @AA-Turner, this is a good solution!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Documentation of source-read event: "docname" can be None
3 participants