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.

Sign up for GitHub

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

Jump to bottom

Remove deprecations for sphinx 1.8 #57

Merged
merged 4 commits into from
Jan 15, 2019
Merged

Remove deprecations for sphinx 1.8 #57

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1b6585a
Remove deprecations for sphinx 1.8
stsewd Jan 3, 2019
5881848
Remove test deprecations
stsewd Jan 3, 2019
3c21236
Revert "Remove test deprecations"
stsewd Jan 3, 2019
0ebecf8
Keep old code
stsewd Jan 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions readthedocs_ext/readthedocs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,11 @@ def finalize_media(app):
# Pull project data from conf.py if it exists
context = app.builder.config.html_context
STATIC_URL = context.get('STATIC_URL', DEFAULT_STATIC_URL)

app.builder.script_files.append(
'%sjavascript/readthedocs-doc-embed.js' % STATIC_URL
)
js_file = '{}javascript/readthedocs-doc-embed.js'.format(STATIC_URL)
if sphinx.version_info < (1, 8):
app.add_javascript(js_file)
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to probably keep the logic the same as the existing logic, since there could be subtle changes in how this works across Sphinx versions.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've changed it, but we have the deprecation from 1.7 actually, https://www.sphinx-doc.org/en/1.7/extdev/appapi.html#sphinx.application.Sphinx.add_javascript was added in 0.5

else:
app.add_js_file(js_file)
stsewd marked this conversation as resolved.
Show resolved Hide resolved


def update_body(app, pagename, templatename, context, doctree):
Expand Down Expand Up @@ -90,7 +91,10 @@ def update_body(app, pagename, templatename, context, doctree):
pass

if inject_css and theme_css not in app.builder.css_files:
app.builder.css_files.insert(0, theme_css)
if sphinx.version_info < (1, 8):
app.add_stylesheet(theme_css)
Copy link
Member

Choose a reason for hiding this comment

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

Same here, I don't see a reason to change this logic.

else:
app.add_css_file(theme_css)
Copy link
Member

Choose a reason for hiding this comment

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

This may have a different effect since we were adding our .css file as the first element of the list.

So, all the css files added after our could override our own css classes. I'm not sure at what point add_stylesheet and/or add_css_file will add them.

In any case, we should check if inserting it at 0 position is something we really need.

Copy link
Member

Choose a reason for hiding this comment

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

I saying this because I think the order of the list is the order in which the css files are loaded in the HTML, but I'm not sure either.

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't need that for recent projects, users can always pin the extension

#57 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

So, this is a breaking change, right?

If so, we won't be able to upgrade our default version installed by RTD.

Copy link
Member Author

@stsewd stsewd Jan 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

It's breaking only for really old projects (sphinx_rtd_theme < 4.0, 2013), and supposing that they haven't updated the theme (they have the theme pinned to a version less than 4.0). And if they have a custom css rule that could interfere with the theme. I'm not really so worry about this change.

Copy link
Member

Choose a reason for hiding this comment

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

We don't need that for recent projects, users can always pin the extension

Big -1 here. Users should never need to know about our extension, asking them to pin it is definitely terrible UX :)


# This is monkey patched on the signal because we can't know what the user
# has done with their `app.builder.templates` before now.
Expand Down