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

Avoid warning about multiple graphviz directives #804

Merged
merged 2 commits into from
Feb 14, 2022
Merged

Avoid warning about multiple graphviz directives #804

merged 2 commits into from
Feb 14, 2022

Conversation

2bndy5
Copy link
Contributor

@2bndy5 2bndy5 commented Feb 14, 2022

This PR fixes #803 (a priority bug in v4.33.0).

  • remove duplicated config options inherent to sphinx.ext.graphviz. This was causing a warning in sphinx build logs.
  • remove attempt to register graphviz directive. This is needed to allow users to arbitrarily add breathe and sphinx.ext.graphviz to the conf.py extensions list in whatever order.

As stated already in the docs (added in #757), using the dot tool requires that sphinx.ext.graphviz be added to the extensions list in conf.py. Appropriate warnings will get triggered when breathe uses the graphviz directive without adding sphinx.ext.graphviz to the extensions list.

Just to reiterate what is said in the docs:

  • sphinx.ext.graphviz is required for the \dot and \dotfile cmds.
  • :allow-dot-graphs: (an option for doxygenclass, doxygenstruct, doxygenfile, doxygenindex
    autodoxygenfile, and autodoxygenindex directives) is required for the doxygen-generated graphs (like include/inheritance/collaboration graphs), since sphinx.ext.graphviz is required to use the graphviz directive that will render the doxygen-generated graphs. Without specifying :allow-dot-graphs:, the doxygen generated graphs will be ignored (even if sphinx.ext.graphviz is added to the list of used extensions in conf.py).

@gmarull did the right thing by commenting on #757. Otherwise, I would not have been alerted to #803.

Please tag me ( @2bndy5 ) in the future if there are any other problems that arise from the feature introduced by #757. 🤞🏼

@2bndy5
Copy link
Contributor Author

2bndy5 commented Feb 14, 2022

I've been testing these changes on the same sample project used in #757, but with the -W flag asserted.

@2bndy5 2bndy5 marked this pull request as ready for review February 14, 2022 13:32
@2bndy5 2bndy5 marked this pull request as draft February 14, 2022 14:28
@2bndy5 2bndy5 marked this pull request as ready for review February 14, 2022 15:03
@vermeeren vermeeren self-assigned this Feb 14, 2022
@vermeeren vermeeren self-requested a review February 14, 2022 16:38
@vermeeren vermeeren added bug Problem in existing code code Source code regression Something broke that worked in the past labels Feb 14, 2022
@2bndy5
Copy link
Contributor Author

2bndy5 commented Feb 14, 2022

This seems to be high priority given the amount of references in #803

I can't think of any other ways/scenarios to test these changes on. Everything looks good and ready for review. Again, I'm very sorry I didn't catch this before v4.33.0 release.

michaeljones pushed a commit that referenced this pull request Feb 14, 2022
@michaeljones michaeljones merged commit 9f94e24 into breathe-doc:master Feb 14, 2022
@2bndy5 2bndy5 deleted the avoid-multi-graphviz-directives branch February 14, 2022 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Problem in existing code code Source code regression Something broke that worked in the past
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extension Graphviz configuration clashes with built-in Graphviz extension
3 participants