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

Put the rtd extension to the beginning of the list #4054

Merged
merged 2 commits into from May 24, 2018

Conversation

Projects
None yet
4 participants
@stsewd
Member

stsewd commented May 3, 2018

This fix #3715

The problem is because of the sphinx_tabs.tabs extension, which somehow collides with the rtd extension. If the rtd extension is put before the tabs extension on the list of extensions the problem is solved.

I'm not sure if there are other implications with this change, or isn't really relevant for others users that don't use the tabs extension to have the rtd extension to the end or beginning.

@stsewd

This comment has been minimized.

Member

stsewd commented May 3, 2018

I'll try to investigate the tabs extension to see if this fix should be on the rtd-extension project.

@stsewd

This comment has been minimized.

Member

stsewd commented May 3, 2018

I don't know what is causing the problem on the tabs extension side :/ this is the update_context function https://github.com/djungelorm/sphinx-tabs/blob/a7513f1be4d63c5c7479349842e9522ab61b907a/sphinx_tabs/tabs.py#L214

@stsewd

This comment has been minimized.

Member

stsewd commented May 3, 2018

Maybe something to do with the doctree.walk(visitor) code 🤔

@@ -134,6 +134,6 @@ else:
# Add custom RTD extension
if 'extensions' in globals():
extensions.append("readthedocs_ext.readthedocs")
extensions.insert(0, "readthedocs_ext.readthedocs")

This comment has been minimized.

@humitos

humitos May 8, 2018

Member

I don't understand either why in this way it works (or not), but we definetely need to add a comment here explaining why we are changing this since we don't have an easy way to write a test.

This comment has been minimized.

@stsewd

stsewd May 8, 2018

Member

Also, there isn't a good way of explaining it 😁, maybe a link to this PR is enough?

@davidfischer

This comment has been minimized.

Contributor

davidfischer commented May 8, 2018

I don't know what is causing the problem on the tabs extension side

I dug a little deeper here and I believe it is the fact that context["css_files"] and app.builder.css_files get out of sync.

The sphinx tabs extension tries to optimize a bit by removing the JS/CSS if it doesn't appear to be used. When it does so, it modifies context["css_files"] directly. While sphinx provides an add_stylesheet API, there is nothing to remove them. In our extension, we modify app.builder.css_files which is deprecated in favor of app.add_stylesheet.

One other note is that add_stylesheet is supposed to add the CSS file to every page while sphinx-tabs is trying to remove it on a single page.

@davidfischer

This comment has been minimized.

Contributor

davidfischer commented May 8, 2018

@djungelorm, I figure you might be interested in this.

@ericholscher

This comment has been minimized.

Member

ericholscher commented May 22, 2018

This seems like a good thing that we should probably be doing anyway, so I'm +1. It does feel like something that might cause other unindented issues, but such is life :)

@ericholscher ericholscher merged commit 72e6974 into rtfd:master May 24, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ericholscher

This comment has been minimized.

Member

ericholscher commented May 24, 2018

Looked over this, and think it's good to merge. We will see if it causes other interactions though.

I think the better long term fix is to insert our CSS files using the new Sphinx API, but I don't know how easy that will be to do in the extension.

@stsewd stsewd deleted the stsewd:fix-rtd-extension-place branch May 24, 2018

@stsewd

This comment has been minimized.

Member

stsewd commented May 24, 2018

The api has the limitation that it doesn't allow to insert values into a specific place, but only to the end. And I don't think using the other method would fix this problem :/ behind scenes it does the same.

@ericholscher

This comment has been minimized.

Member

ericholscher commented May 24, 2018

So is the problem that sphinx-tabs is copying the context, so that when we edit it, we are both using a different context object?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment