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

Make build process robust against missing nodes #9921

Open
astrojuanlu opened this issue Dec 1, 2021 · 1 comment
Open

Make build process robust against missing nodes #9921

astrojuanlu opened this issue Dec 1, 2021 · 1 comment
Labels
type:enhancement enhance or introduce a new feature

Comments

@astrojuanlu
Copy link
Contributor

Is your feature request related to a problem? Please describe.
Projects often pick extensions that introduce new nodes, and at the same time they try to build output other than HTML, perhaps provided by another extension.

A specific case of this is #9919, which uses Sphinx-diagrams (adding a new diagram node) combined with sphinxcontrib-confluencebuilder, which doesn't understand the concept of diagrams. Other recent examples include nbsphinx + pdf build.

In all these cases, the build crashes, even if -W hasn't been used.

Describe the solution you'd like
It would be nice to to produce an empty box or generic node with a warning in these cases, rather than failing the build. At least most of the documentation could be build.

Extra points if the warning says which extension registered the node, so the issue can be opened in the right repository straight away.

Describe alternatives you've considered
Naturally, one alternative is not doing anything. I think the current status is problematic, but I haven't weighed the cost of changing it.

This behavior could be made configurable, but I guess it can be treated like any other Sphinx warning.

@astrojuanlu astrojuanlu added the type:enhancement enhance or introduce a new feature label Dec 1, 2021
@jdknight
Copy link
Contributor

jdknight commented Dec 4, 2021

To give an extension developer's perspective, raising an NotImplementedError exception appears to be the "good practice" approach when first diving into Sphinx. When looking at Sphinx's implementation for reference, it is used throughout the internal implementation and replicating methods like this appear to be the "right" way forward:

    def unknown_visit(self, node: Node) -> None:
        raise NotImplementedError('Unknown node: ' + node.__class__.__name__)

But if NotImplementedError generates an exception, a user thinks of this as a generic Sphinx issue and then reports to this issue tracking (which I'm sure that's how #9919 came to light), I can agree that there is an issue here.

Personally, I do not think extension writers (in most cases) should even need to implement an unknown_visit hook -- if the sole purpose is to report that it is an unsupported node. From what I can tell, docutils already does this (even in v0.14). Would it be possible to just drop all existing Sphinx-managed translators from defining their own unknown_visit call? This could help prevent future extensions created from replicating these calls.

While this can avoid new extensions from introducing this call, this still results in an exception being raised. Since all Sphinx translators appear to inherit from SphinxTranslator, an unknown_visit call could be defined here and a warning message could be generated. This appears to be what texinfo does already. And as @astrojuanlu mentions, I would agree that -W use case would still be desired. I assume one of the main reasons why NotImplementedError was introduced in these translators was to ensure that Sphinx's unit testing would fail of an unknown node type (dealing with possibly new docutils implementation and checking for nodes changes that needs to managed).

jdknight added a commit to jdknight/sphinx that referenced this issue Dec 5, 2021
Removes the need for various translators from raising a
`NotImplementedError` exception when missing support for a specific node
type. docutils will already raise [1][2] a `NotImplementedError`
exception for these cases. This help reduce the implementation inside
Sphinx as well as prevents the possible undesired replication of
unknown-node handling with third-party extensions [3].

In most cases, generating a warning message for an unsupported node type
can be preferred. Providing an indication that a node is not supported
can be easier for a user of Sphinx to understand a limitation of a
builder over a generic "not implemented" exception. This commit takes
the logging call which is already used by `texinfo` and applies it to
the `SphinxTranslator` base class -- which any Sphinx translator
implementation can use.

[1]: https://repo.or.cz/docutils.git/blob/d169015ee0f412cffd69b33654d8a119d99bc0f3:/docutils/nodes.py#l2048
[2]: https://repo.or.cz/docutils.git/blob/53716a13b48128af6045139d3cd2909f61e7ed8e:/docutils/nodes.py#l1897
[3]: sphinx-doc#9921

Signed-off-by: James Knight <james.d.knight@live.com>
AA-Turner pushed a commit to AA-Turner/sphinx that referenced this issue Dec 29, 2021
Removes the need for various translators from raising a
`NotImplementedError` exception when missing support for a specific node
type. docutils will already raise [1][2] a `NotImplementedError`
exception for these cases. This help reduce the implementation inside
Sphinx as well as prevents the possible undesired replication of
unknown-node handling with third-party extensions [3].

In most cases, generating a warning message for an unsupported node type
can be preferred. Providing an indication that a node is not supported
can be easier for a user of Sphinx to understand a limitation of a
builder over a generic "not implemented" exception. This commit takes
the logging call which is already used by `texinfo` and applies it to
the `SphinxTranslator` base class -- which any Sphinx translator
implementation can use.

[1]: https://repo.or.cz/docutils.git/blob/d169015ee0f412cffd69b33654d8a119d99bc0f3:/docutils/nodes.py#l2048
[2]: https://repo.or.cz/docutils.git/blob/53716a13b48128af6045139d3cd2909f61e7ed8e:/docutils/nodes.py#l1897
[3]: sphinx-doc#9921

Signed-off-by: James Knight <james.d.knight@live.com>
@AA-Turner AA-Turner added this to the some future version milestone Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement enhance or introduce a new feature
Projects
None yet
Development

No branches or pull requests

3 participants