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

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

Merged
merged 2 commits into from May 24, 2018

Conversation

@stsewd
Copy link
Member

@stsewd 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
Copy link
Member Author

@stsewd 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.

Loading

@stsewd
Copy link
Member Author

@stsewd 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

Loading

@stsewd
Copy link
Member Author

@stsewd stsewd commented May 3, 2018

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

Loading

@@ -134,6 +134,6 @@ else:

# Add custom RTD extension
if 'extensions' in globals():
extensions.append("readthedocs_ext.readthedocs")
extensions.insert(0, "readthedocs_ext.readthedocs")
Copy link
Member

@humitos humitos May 8, 2018

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Member Author

@stsewd stsewd May 8, 2018

Choose a reason for hiding this comment

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

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

Loading

@davidfischer
Copy link
Contributor

@davidfischer 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.

Loading

@davidfischer
Copy link
Contributor

@davidfischer davidfischer commented May 8, 2018

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

Loading

@ericholscher
Copy link
Member

@ericholscher 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 :)

Loading

@ericholscher ericholscher merged commit 72e6974 into readthedocs:master May 24, 2018
1 check passed
Loading
@ericholscher
Copy link
Member

@ericholscher 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.

Loading

@stsewd stsewd deleted the fix-rtd-extension-place branch May 24, 2018
@stsewd
Copy link
Member Author

@stsewd 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.

Loading

@ericholscher
Copy link
Member

@ericholscher 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?

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants