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

fix: Only add external version warning nodes on documents #114

Merged
merged 5 commits into from Sep 14, 2022
Merged

fix: Only add external version warning nodes on documents #114

merged 5 commits into from Sep 14, 2022

Conversation

elenakrittik
Copy link
Contributor

@elenakrittik elenakrittik commented Sep 12, 2022

For some reaosn, Sphinx emits doctree-resolved not only for documents (in our case it was emitted with docutils.nodes.bullet_list as doctree), which causes external version warning to appear in unexpected places (sidebar in our case). This PR adds simple isinstance() check which ensures that doctree is sphinx.addnodes.document, eliminating the above issue.

Resolves #113

@humitos
Copy link
Member

humitos commented Sep 12, 2022

Hrm... Interesting. I think I've experienced this with out own blog recently where the banner was added to multiple places:

Screenshot_2022-09-12_15-50-33

@ItsAleph Is this the issue that you were experimenting as well?

@onerandomusername
Copy link

@ItsAleph Is this the issue that you were experimenting as well?

@humitos We had something similar, but not quite the same as yours. (I'm working on the same project as @ItsAleph)
image

@elenakrittik
Copy link
Contributor Author

elenakrittik commented Sep 12, 2022

Yeah, i had similar issue, this warning was injected into main page, and also in sidebar.
Above

@humitos humitos requested a review from stsewd September 12, 2022 16:53
Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks.

I'm requesting review from another developer as well, just to have some extra eyes as backup :)

readthedocs_ext/external_version_warning.py Outdated Show resolved Hide resolved
Copy link
Member

@stsewd stsewd left a comment

Choose a reason for hiding this comment

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

From sphinx's typehints, it's said that these are documents

https://github.com/sphinx-doc/sphinx/blob/2f1d92773cc6e2857c6d4a0fd30caecbbfd5d4b3/sphinx/environment/__init__.py#L567-L582

so, this might be a sphinx bug... I'll see if I can replicate with other versions of sphinx.

readthedocs_ext/external_version_warning.py Outdated Show resolved Hide resolved
@stsewd
Copy link
Member

stsewd commented Sep 12, 2022

Ok, so from sphinx-doc/sphinx#3796 and some extensions https://github.com/search?q=org%3Asphinx-contrib+resolve_references&type=code.

Sphinx always does use resolve_references with a document node, but that function can be used by extensions, and they may not pass a document node.

elenakrittik and others added 2 commits September 13, 2022 07:36
Co-authored-by: Santos Gallegos <stsewd@proton.me>
Co-authored-by: Santos Gallegos <stsewd@proton.me>
readthedocs_ext/external_version_warning.py Outdated Show resolved Hide resolved
Co-authored-by: Santos Gallegos <stsewd@proton.me>
@stsewd stsewd merged commit 9e75356 into readthedocs:main Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Is it possible to exclude some parts of extension?
4 participants