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

adding rtd translator #95

Merged
merged 1 commit into from
Feb 27, 2020
Merged

adding rtd translator #95

merged 1 commit into from
Feb 27, 2020

Conversation

choldgraf
Copy link
Collaborator

@choldgraf choldgraf commented Feb 24, 2020

Adding a readthedocs translator for sites that build on RTD.

I'm not sure how to test this other than to see whether it looks correct on readthedocs... @jorisvandenbossche are you OK with a merge and spot-check afterwards?

The fix was inspired by comments here: readthedocs/readthedocs.org#6690

@jorisvandenbossche
Copy link
Member

@choldgraf yes, fine with just merging and relying on other projects using it as "test".

But, can you also add the additional builder as done here: https://github.com/readthedocs/sphinx-hoverxref/blob/master/hoverxref/extension.py#L116 ("readthedocsdirhtml", I think that is for if you enable a "download html" version), and also add a comment explaining it briefly (or pointing to the issue)?

@choldgraf
Copy link
Collaborator Author

ok cool - just added them both, let's see if RTD builds it

@choldgraf choldgraf merged commit 3df0dff into pydata:master Feb 27, 2020
@choldgraf choldgraf deleted the rtd branch February 27, 2020 17:50
@choldgraf
Copy link
Collaborator Author

@jorisvandenbossche
Copy link
Member

Thanks!

@humitos
Copy link

humitos commented Apr 4, 2020

FYI, I changed the way that we are handling this in sphinx-hoverxref for a more permissive way. Instead of overriding the translator completely, I'm just adding the behavior that I need by creating a new class dynamically and inheriting from the translator define by the user (or extension) plus my mixin class with the behavior I need.

This allowed me to make my extension compatible with your theme. I just tested this theme with my extension and they both work nicely together!

So, if you think that you can be compatible with other HTML5Translator that the user could define, you may want to follow a similar pattern to avoid the same problem that I had with your theme.

See the PR at readthedocs/sphinx-hoverxref#42

@jorisvandenbossche
Copy link
Member

jorisvandenbossche commented Apr 4, 2020

@humitos thanks! that sounds interesting, that should indeed allow to not have to "hardcode" such support. I moved your comment to an new issue -> #143

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

Successfully merging this pull request may close these issues.

3 participants