-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Don't depend of enabled pdf/epub to show downloads #5502
Conversation
@@ -258,32 +256,25 @@ def get_subdomain_url(self): | |||
def get_downloads(self, pretty=False): | |||
project = self.project | |||
data = {} | |||
if pretty: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code was duplicates, just refactored it to something simpler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good fix. I wonder if there are other pieces of logic around this that need to change with the config files.
Apart from the |
self.slug, | ||
) | ||
if project.has_htmlzip(self.slug): | ||
data[prettify('HTML')] = project.get_production_media_url( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data[prettify('HTML')] = project.get_production_media_url( | |
data[prettify('HTMLZIP')] = project.get_production_media_url( |
I believe this should be HTMLZIP
instead of HTML
. The template that displays these links checks for dict.htmlzip
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I just realized that when prettify was set, we named this to html instead of htmlzip. I need to see if we use this in the footer api, or maybe it's safe to rename the template.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We currently use the name HTML
in the footer api and on the theme https://github.com/rtfd/sphinx_rtd_theme/blob/a49a812c8821123091166fae1897d702cdc2d627/sphinx_rtd_theme/versions.html#L17-L21
So, it's safe to rename this in the template
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In readthedocs#5502 we refactored the function but we used to depend on `pretty` to change the name of the key from `html` to `htmlzip`, we use `html` in the footer and in the theme. So, it's safe to change the value in the template. See https://github.com/rtfd/readthedocs.org/pull/5502/files#r272787188
Fixes #5497
We just need to rely if the epub/pdf exists, we already do this with the localmedia download.