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

Pass-through configuration directly to Thebe #17

Open
Tracked by #39
choldgraf opened this issue Oct 22, 2020 · 7 comments · May be fixed by #25
Open
Tracked by #39

Pass-through configuration directly to Thebe #17

choldgraf opened this issue Oct 22, 2020 · 7 comments · May be fixed by #25
Labels

Comments

@choldgraf
Copy link
Member

choldgraf commented Oct 22, 2020

Description

For any configuration that exists in Thebe, we should simply pass it through directly to thebe rather than having our own configuration key name for it and translating it.

Benefit

This way we standardize on a single set of configuration keys, and can simply refer to the thebe docs for the full reference.

Implementation

Here is where thebe documents its configuration: https://thebe.readthedocs.io/en/latest/configure.html#

and we translate and configure thebe in this package here:

thebe_html_config = f"""
<script type="text/x-thebe-config">
{{
requestKernel: true,
binderOptions: {{
repo: "{org}/{repo}",
ref: "{branch}",
}},
codeMirrorConfig: {{
theme: "{codemirror_theme}",
mode: "{cm_language}"
}},
kernelOptions: {{
kernelName: "{kernel_name}",
path: "{path_to_docs}{str(Path(docname).parent)}"
}},
predefinedOutput: true
}}
</script>

Note now we are "manually" translating many things into thebe's configuration. Instead of this, we should just accept something like thebe_config, which would be a dictionary that would directly map onto Thebe configuration.

@akhmerov
Copy link
Contributor

Coming here from #19, this seems like the preferred approach; is there anything blocking it?

@choldgraf
Copy link
Member Author

Nope, just hours in the day, I am happy to review a pr 🙂

@akhmerov
Copy link
Contributor

I can see what I can do. What's your preferred approach to backwards compatibility? A separate config variable + deprecation cycle? Moving fast and breaking things? Another option?

@choldgraf
Copy link
Member Author

Good question. Maybe we could add a key for "thebe_config_raw = True" or something, and deprecate over a cycle or two?

@choldgraf
Copy link
Member Author

TBH I'd also be fine just changing it now, as long as there wasn't a strong reason to keep the current behavior

@akhmerov
Copy link
Contributor

My preferred approach would be indeed to change the interface directly. The arguments why it wouldn't be too harmful are:

  • v0.0.x should be enough of a warning
  • the package users are likely capable of either pinning to the older version or updating to the new one

The only argument against I can see is the requirement of sphinx-thebe>=0.0.6 in jupyter-book.

@choldgraf
Copy link
Member Author

I good point, we should pin jupyter book to ~= 0..0.* first

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

Successfully merging a pull request may close this issue.

2 participants