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

Refactor directive addition #780

Merged
merged 3 commits into from Jan 22, 2022

Conversation

jakobandersen
Copy link
Collaborator

@jakobandersen jakobandersen commented Dec 29, 2021

Based on #775 it turns out that they way Breathe uses add_directive is not strictly adhering to the interface: the argument must be a class derived from docutils.parsers.rst.Directive (https://www.sphinx-doc.org/en/master/extdev/appapi.html#sphinx.application.Sphinx.add_directive).
This was not done because the Breathe directives needs access to some objects created during setup-time.
This PR uses a different hax: use env.temp_data to smuggle in those objects, and set them every time a new document is processed, via the source-read event. Thereby the directives can be added directly.

It's still ugly but I haven't found a prettier way to do it.

References:

Copy link
Collaborator

@michaeljones michaeljones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch. Sorry that my efforts to be in control of object construction has led to difficulties with the Sphinx API. I would rather we passed it creator functions instead but that seems hard to wrangle.

Happy for you to merge this whenever you like.

@jakobandersen jakobandersen merged commit 3958c34 into breathe-doc:master Jan 22, 2022
@jakobandersen jakobandersen deleted the add_directive branch January 22, 2022 11:42
@jakobandersen
Copy link
Collaborator Author

Great catch. Sorry that my efforts to be in control of object construction has led to difficulties with the Sphinx API. I would rather we passed it creator functions instead but that seems hard to wrangle.

No worries, this is just a different hax :-). I agree the API should be more flexible, though that change in Sphinx would probably anyway take a few years with deprecation and then the actual change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Translate non-default directives (breathe extension) MyST crashes without error message (breathe extension)
2 participants