-
Notifications
You must be signed in to change notification settings - Fork 35
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
Add injection of HTML to a signal #13
Conversation
def finalize_media(app): | ||
""" | ||
Point media files at our media server. | ||
""" |
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're not using pep257 on the extension yet, though we should. This single line comment should be on one line
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.
Would be easy to enable on this repo at least, since it's tiny. I can work on that.
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.
Not a huge priority, I opened #14 to deal with it later. I'm sure there will be some cleanup required as well
Just noted some updates to docstrings to match pep257. We should implement our tox config here eventually as well. I'll open an issue on that. This all seems reasonable, without testing it directly. 👍 |
9a7dafd
to
d1c0389
Compare
Either way, if the search stuff is ready to go, we can merge it, and then make it gets migrated into this PR. We could merge this one first also, and update the search PR, either way. |
I have the search stuff pointing to the |
Sounds good. I'm around today if we want to get the search & extension stuff deployed so we can catch issues with it over the weekend. |
{% if page_source_suffix %} | ||
<!-- Add page-specific source suffix, which must exist in the page data, not global --> | ||
<script type="text/javascript"> | ||
READTHEDOCS_DATA['source_suffix'] = {{ page_source_suffix }} |
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 should be quoted, and should be changed in the data file template as well. Does this exist here to ensure the change for projects built with a readthedocs-data.js
but could be out of date?
I added this change to the template in #17 either way.
Edit: sorry, just realized only new projects will get this either way, as it's the template include. It doesn't seem we should implement this change here at all, no?
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.
The data file is per project, not per-page.
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.
Roger, this just needs to be quoted then. I'll reflect the change in #17
return content | ||
|
||
app.builder.templates.render = rtd_render | ||
HAS_MONKEYPATCH = True |
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.
The global state tracking here seems unneeded, what about:
if app.builder.templates.render != rtd_render:
app.builder.templates.render = rtd_render
Or is there a reason to avoid setting this again here?
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.
Because we need to capture the old render function to be able to wrap it. We can't create the new render function without having a reference to the old one, so we have to know whether app.builder.templates.render
is the original or our version, which I believe requires a global, no?
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, this is capturing old_render
, which will be in scope in the rtd_render
closure. If testing for if render == rtd_render
is enough here, then the HAS_MONKEYPATCH
should be superfluous
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.
How do you test for rtd_render
without defining it though? And to define it you have to have imported render
already, and wrapped it?
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.
Sorry, by global, I was only referring to the global HAS_MONKEYPATCH
, the rest looks fine, assuming we can add a test for app.builder.templates.render != rtd_render
without causing problems.
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.
Right, but we can't wrap it multiple times, because we're altering the output. I don't know a better way to do this? redefining rtd_render
makes it a new function, so there's no way to know if it's the one that we set or not without global state, unless I'm missing something?
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.
Oh, I see what you mean, on second pass, rtd_render
is redefined. I think storing this information on the object we're monkey patching is more useful. This could be either with a pattern similar to mocking:
class WrappedCallable(object):
def __init__(self, fn):
print('WRAPPING')
self.fn = fn
def __call__(self, *args, **kwargs):
print('PRE')
self.fn(*args, **kwargs)
print('POST')
def foo():
print 'FOO'
old_foo = foo
if not isinstance(foo, WrappedCallable):
foo = WrappedCallable(foo)
if not isinstance(foo, WrappedCallable):
foo = WrappedCallable(foo)
foo()
or simply storing this on the function object:
app.builder.templates.render._is_patched = True
or something
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.
Yea, I went ahead and threw it on the app
, since I don't trust the templates object not to be re-initialized.
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.
Put what on app
? I was saying that instead of tracking HAS_MONKEYPATCH
in our module, we should be tracking that state on the object we're passing. The first approach above tracks the function we've patched already, though we can also do this with the second approach -- that is, something like:
if not getattr(app.builder.templates.render, '_wrapped_call'):
old_render = app.builder.templates.render
def rtd_render(...):
...
context = old_render(...)
...
rtd_render._wrapped_call = old_render
app.builder.templates.render = rtd_render
This is just an example, its untested.
This gives users the ability to reclaim app.builder.templates.render._wrapped_call
, if something is completely broken with our patching. It also provides a truthier test if the object is actually patched.
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.
gotcha, that makes sense.
Also, this gets rid of my changes that were just moved in for in-doc search in #16 |
Just thinking of this now. The extra head template should output all of this into First,
But, also, I believe we need much of this javascript loaded earlier. 1: https://html.spec.whatwg.org/multipage/semantics.html#the-link-element |
Opened readthedocs/readthedocs.org#2021 to address some potential issues stemmed from javascript load order. |
Add injection of HTML to a signal
This is because there's no way to consistently add things into
<head>
via Sphinx. The doctree node that is used internally only represents the body of the document, and theme's are responsible for wrapping it with proper HTML. There is no standard for block naming and other issues within the<head>
-- so we are left with inserting HTML into the bottom of the body element as the most consistent way to override Sphinx's HTML content...Fixes #12