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

Switch templates via query string parameters #201

Closed
wants to merge 3 commits into from

Conversation

jtpio
Copy link
Member

@jtpio jtpio commented Jun 6, 2019

A proof of concept for switching templates on the fly via the notebook metadata and query string parameters.

Related to #188 and #105.

  • Rename the cli parameter template to templates to pass a list of available templates (instead of a single one)
  • Fetch template from the notebook metadata
  • Fetch template from the query string parameters
  • (tests are not fixed yet)

One thing I'm not super happy with is the need to sort the nbconvert_template_paths list before passing it to the nbconvert exporter.

Also this approach might be completely invalidated if / when the template mechanism is changed for the next release.

template-url

@jtpio jtpio changed the title WIP Switch templates from URL or metadata (do not merge) [WIP Switch templates from URL or metadata (do not merge) Jun 6, 2019
@jtpio jtpio changed the title [WIP Switch templates from URL or metadata (do not merge) [WIP] Switch templates from URL or metadata (do not merge) Jun 6, 2019
@SylvainCorlay
Copy link
Member

Thanks @jtpio!

I wonder if we could drop the list of allowed templates and just let users select any installed template from the url params.

The --template=foobar command-line argument would just select the default template that is used when none is specified with the url parameter.

@martinRenou
Copy link
Member

I wonder if we could drop the list of allowed templates and just let users select any installed template from the url params.

👍

@jtpio
Copy link
Member Author

jtpio commented Jun 7, 2019

Yes absolutely.

@jtpio jtpio force-pushed the multiple-templates branch 2 times, most recently from 45462c5 to 34bf7b1 Compare June 7, 2019 13:23
@jtpio
Copy link
Member Author

jtpio commented Jun 7, 2019

Pushed new changes to:

  • Drop the --templates=["gridstack", "default"] option and instead use all available templates
  • Keep --template option to default to a specific template
  • Assume there is always the default template installed

notebooks/basics.ipynb Outdated Show resolved Hide resolved
@jtpio jtpio force-pushed the multiple-templates branch from 34bf7b1 to 60251f7 Compare June 12, 2019 15:58
@jtpio jtpio changed the title [WIP] Switch templates from URL or metadata (do not merge) Switch templates via query string parameters Jun 12, 2019
@jtpio jtpio force-pushed the multiple-templates branch from 60251f7 to 9f0a0e1 Compare June 17, 2019 10:50
@jtpio jtpio force-pushed the multiple-templates branch 2 times, most recently from 58f3a47 to c554241 Compare June 18, 2019 14:58
templates = self.nbconvert_template_paths
if self.voila_configuration.template:
# prioritize paths based on the template name if specified
templates = sorted(self.nbconvert_template_paths, key=lambda p: -(template_name in p))
Copy link
Member

Choose a reason for hiding this comment

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

This sorted call feels bad to me... I suppose we should collect the right template paths for each request. See #258

Copy link
Member

Choose a reason for hiding this comment

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

I suppose something cleaner and safer would be templates = collect_template_paths(..., template_name=...)

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree it's not very elegant.
Would need to check how nbconvert handles templates that make use of inheritance.

@cailiang9
Copy link
Contributor

cailiang9 commented Jun 14, 2020

It is cool but I think it is not a proper place to specify notebook specific template parameters.
Because the render link could be open to common users directly.
Allowing specify template from common users is unsafe.
A better place to specify notebook specific template is notebook metadata (edit -> edit metadata in jupyter).

@maartenbreddels
Copy link
Member

Allowing specify template from common users is unsafe.

I think both options should be possible, but I agree that a user specifying a template should be possible to disable.

@maartenbreddels
Copy link
Member

Closes #188

@jtpio
Copy link
Member Author

jtpio commented Oct 29, 2020

Fixed by #637

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.

5 participants