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

Bryanv/3962 server templates #4176

Merged
merged 7 commits into from
Apr 23, 2016
Merged

Bryanv/3962 server templates #4176

merged 7 commits into from
Apr 23, 2016

Conversation

bryevdv
Copy link
Member

@bryevdv bryevdv commented Apr 14, 2016

This PR adds initial support for overriding the default FILE template for Bokeh apps with a custom user-specified template.

@bryevdv
Copy link
Member Author

bryevdv commented Apr 14, 2016

@birdsarah Do you think you could add some minimal template and styling around one of the apps (maybe weather or movies) ? Then we can point to it explicitly in the docs.

@bryevdv
Copy link
Member Author

bryevdv commented Apr 14, 2016

@bokeh/dev @havocp I feel like there might be some conceptual fuzziness between handlers, applications, and commands. Mostly this its just about who is responsible for what, and how best to generally handle combinations that "don't make sense". We seem to be getting lots of cases like this where it makes sense for "at most one handler to do XYZ" maybe there is some pattern that would help encapsulate this better.

@bryevdv
Copy link
Member Author

bryevdv commented Apr 14, 2016

E.g., maybe we can find a declarative way to tag handler classes by the optional features they might provide. This could help both with checking the "at most one..." cases but also help with warnings or alternate code paths (e.g a bokeh html command running an app with static resources)

@havocp
Copy link
Contributor

havocp commented Apr 14, 2016

each handler modifies the document, right? they are allowed to overwrite each other. I think the conceptually-pure approach here would be that if two handlers both set doc.template, the second one just wins. But making it an error is perhaps more useful/practical, if we believe (and this is probably true) that two templates represents a mistake.

@@ -0,0 +1,2 @@

ldsfjlaskgjgldjfg
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this intended to be committed?

Copy link
Member Author

Choose a reason for hiding this comment

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

hah no it was not

Copy link
Contributor

Choose a reason for hiding this comment

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

this code is genius though

@bryevdv
Copy link
Member Author

bryevdv commented Apr 14, 2016

Sure, that approach occurred to me to, it's certainly consistent with e.g., general python namespace behavior. But even if we go that route, practically speaking I think we need to at least warn if it happens.

HERE IS SOME CUSTOM STUFF
</div>
{{ plot_div|indent(8) }}
{{ plot_script|indent(8) }}
Copy link
Contributor

Choose a reason for hiding this comment

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

to me this is too obscure, I'd love to see it simplified down to {{bokeh_head}}, {{document}}, and {{title}} perhaps (or other more obviously-meaningful things). I believe it's possible to do that without breaking the ability to also do the above more detailed breakdown. But it could be its own PR for sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

other than clarity, making this less granular would mean fewer apps break if the implementation details ever change...

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes sorry I should have specified, I am definitely wanting to spread things out over a few independent PRs. This one make a minimal change to add a pretty important capability, do intend to refine it more.

@havocp
Copy link
Contributor

havocp commented Apr 14, 2016

A kind of weird thing is that people can change curdoc().template in a callback, which would only ever do anything if the same session was reloaded, so it would probably be pointless. Not sure we could/should bother to do anything about it.

if not isinstance(template, jinja2.Template):
raise ValueError("Document templates must be Jinja2 Templates")
self._template = template
# TODO (bev) template change eventing?
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't think of where change notification would be practically useful right now, fwiw

@havocp
Copy link
Contributor

havocp commented Apr 14, 2016

should bokeh html use the template too?

@bryevdv
Copy link
Member Author

bryevdv commented Apr 14, 2016

Well so that's my question, is a template a property of the Document? or of an Application? Could make template private, only updatable by handlers using private APIs

@bryevdv
Copy link
Member Author

bryevdv commented Apr 14, 2016

Yes bokeh html should use template for sure

@havocp
Copy link
Contributor

havocp commented Apr 14, 2016

Either way can probably be made to work... if the static resources are on application and not doc, maybe it makes sense to keep it with those. Document could try to be more only the thing that's modified on the fly and synced over the websocket, Application would provide the HTML "container" around the document ?
The reason i was thinking it had to be on Document was so the various output modes (server, standalone html, etc.) would just work, but as we discussed when doing the static resources PR we can probably solve that anyway when putting this stuff on Application.

@bryevdv
Copy link
Member Author

bryevdv commented Apr 14, 2016

A kind of weird thing is that people can change curdoc().template in a callback, which would only ever do anything if the same session was reloaded

For sure, tho being able to set it programmatically at the end of an app script seems useful. I could imagine ways to warn about this, though... Setting an _initialized flag as the last step of creating a doc in a session, and having the template setter check that and warning if it is changed once initialized.

@bryevdv
Copy link
Member Author

bryevdv commented Apr 22, 2016

@bokeh/dev I think there may eventually be tweaks to make to the implementation, but I think the user-facing interface reasonable as-is. I've simplified some things as described above (i.e., handlers just update templates in order without error) I'd like to merge this PR as it is and make incremental improvements.

@bryevdv
Copy link
Member Author

bryevdv commented Apr 22, 2016

@bokeh/dev I will plan to merge this tomorrow sometime absent objection.

@bryevdv bryevdv merged commit 0984b47 into master Apr 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom web templates with bokeh serve
3 participants