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

linkcheck errors on unknown confluence_* directives and roles #927

Closed
adamtheturtle opened this issue Mar 18, 2024 · 6 comments
Closed

linkcheck errors on unknown confluence_* directives and roles #927

adamtheturtle opened this issue Mar 18, 2024 · 6 comments
Milestone

Comments

@adamtheturtle
Copy link

  • Use any confluence_* directive
  • Run the linkcheck builder (e.g. python -m sphinx -W -b linkcheck source build/linkcheck -E -a).
  • See errors about "Unknown directive type"s.

This also applies to the sphinxcontrib.spelling builder.

I do not expect to see errors.
I believe that this is because the roles and directives are only added in the case that the builder is the Confluence builder:

https://github.com/sphinx-contrib/confluencebuilder/blob/bc2dbe66eb4865b017d5366e2c796db32e16fcf8/sphinxcontrib/confluencebuilder/__init__.py#L298C5-L298C30

My initial thought was to use .. only or .. ifconfig, but these did not work, and I believe that this is explained at sphinx-doc/sphinx#4224 (comment):

Unfortunately, only and ifconfig works on writing phase. So the contents of these directives are parsed on reading phase. This is specification of these directives.

Advice welcome on how best to use these directives with the link checker / spell checker.

@jdknight
Copy link
Member

This extension disabled the automatic registration of custom directives/roles due to the reason that a combination that some third-party builders, Sphinx and docutils could generate NotImplementedError exceptions when a custom directive/role would produce a node type that builders would not support (e.g. #505). For example, if confluence_excerpt is set, running with an html builder would generate an exception.

We did get a change into Sphinx (v4.4+) which defaulted Sphinx to generate warnings over exceptions for unknown nodes. Since it has been some time since this was merged in, we might be able to re-introduce the registration of these custom directives/roles again by default. This will of course result in generating "unknown node" warnings over unknown directive/role warnings for an active translator. Maybe this is not an issue for builders like linkcheck since maybe there is no translator stage (do not know if this is true)? Although if the builder does see the unknown nodes and generates warnings, the use case would still fail with the -W argument set. To counter that last point (and as you have referenced), the unknown node warning is in the writer stage and these warnings can be prevented using features like only directive.

So, if we:

  • Try moving the Confluence-specific init check to just before we do some custom extension injection;

  • Check that a run of linkcheck does not generate a warning; and,

  • See if this content does not cause any issue (and better yet, is triggered with linkcheck):

    .. confluence_expand::
    
        Some link: https://example.com/this-is-broken.html
    

Maybe the change noted in the first point would be sane to make.

However, a worry is that users with existing documentation that use a mix of confluence and other builders (such as html), may now have new warnings generated in their documentation that did not have warnings before (if they didn't use directives like only). This is less than ideal for users who may use this custom directives/roles that are only expected to be used by this extension, and adding the only directive does seem bloaty and not pretty sometimes. So maybe a better way forward is to maybe generate dummy directives/roles that "do nothing" when using a non-Confluence builder... but is the really a "proper" approach. For example, if a document had the following, no warning would be generated but in an HTML builder, the content may "look" wrong:

See :jira:`TEST-123` for more details.

(see also: #868)

Will ponder this a bit. Welcome any comments.

@adamtheturtle
Copy link
Author

Thank you for the quick and again very detailed response.

I think you have more context than I do, and I don't have a great understanding of the needs of users who have both html and confluence, but do not use only.

That said, I wonder if either as a permanent or temporary solution, you could expand the builders with which the directives are registered.

Something like:

from sphinx.builders.linkcheck import CheckExternalLinksBuilder
try:
    from sphinxcontrib.spelling.builder import SpellingBuilder
except ModuleNotFoundError:
    acceptable_builders = (ConfluenceBuilder, CheckExternalLinksBuilder)
else:
    acceptable_builders = (ConfluenceBuilder, CheckExternalLinksBuilder, SpellingBuilder)

if not isinstance(app.builder, acceptable_builders):
    return

or, perhaps, exposing those acceptable_builders as a configuration option.

@jdknight
Copy link
Member

A change has been introduced in #936 which permits the registration of directives/roles when it detects a builder that does not have a translator. Hopefully this will help avoid warnings being generated in special builders (such as linkcheck/sphinxcontrib.spelling), while also preventing the risk of exceptions being thrown in non-Confluence builders that have translators (e.g. html).

This change should be available in the next release (v2.5).

In the rare chance that anyone using this change results in an exception, please report it here or another issue. As a workaround, using the option confluence_adv_disable_init = True in a configuration should mitigate the issue until a proper fix has been made.

@adamtheturtle, thanks for mentioning this. Hopefully, this may improve the user experience for everyone. Please let me know if there is anything that has been overlooked.

@jdknight jdknight added the available-next-release Merged into the main branch but waiting for PyPI release label Mar 30, 2024
@jdknight jdknight added this to the 2.5 milestone Mar 30, 2024
@adamtheturtle
Copy link
Author

Thank you @jdknight ! Nothing is overlooked AFAIK. I really appreciate how thorough and thoughtful you are with this project.

However, if you'd like to support actual link checking, you can use something like the following as the role for confluence_link, when the link checker builder is running:

from docutils.nodes import Node
from docutils.parsers.rst.states import Inliner
from docutils.utils import SystemMessage

def link_role(
    role: str,
    rawtext: str,
    text: str,
    lineno: str,
    inliner: Inliner,
) -> tuple[list[Node], list[SystemMessage]]:
    link_text = text
    link_url = text
    node = nodes.reference(rawsource=rawtext, text=link_text, refuri=link_url)
    return [node], []

@jdknight
Copy link
Member

Thanks for mentioning confluence_link. I forgot about the card support added into this extension.

Updated the implementation to register card hrefs into a linkcheck builder with a custom transform (see #938).

@jdknight
Copy link
Member

jdknight commented Apr 1, 2024

v2.5 is now available on PyPI -- marking as closed.

@jdknight jdknight closed this as completed Apr 1, 2024
@jdknight jdknight removed the available-next-release Merged into the main branch but waiting for PyPI release label Apr 1, 2024
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

No branches or pull requests

2 participants