Skip to content

Allow usage of global configuration values for TryExamples directive if provided by user #161

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

Merged

Conversation

agriyakhetarpal
Copy link
Member

Description

This PR updates the TryExamplesDirective class to use the configuration value in conf.py if specified, and otherwise use the "Try it with JupyterLite" text. This way, buttons that are created manually with the

.. try_examples::

directive in Sphinx will receive the same configuration value, and adding a :button_text input across several .. try_examples:: directives to customise the button text will now not be required. The try_examples_global_button_text string will be used by default, instead of the current "Try it with JupyterLite!" text.

To explain this behaviour, an additional sentence has been added to the documentation for the TryExamples directive page.

Closes #157

Footnotes

This is essentially a breaking change of sorts for dependents who are not using a custom :button_text input already, since it will now change the text to what they have set in try_examples_global_button_text (in case they have set that). But it's surely not a very big change and I would consider it as more of an enhancement (cc: @steppi)

@agriyakhetarpal
Copy link
Member Author

Happy to add a CHANGELOG entry for this if needed!

@agriyakhetarpal
Copy link
Member Author

Oops, let me run black and rebase

@steppi
Copy link
Collaborator

steppi commented Mar 29, 2024

Happy to add a CHANGELOG entry for this if needed!

No need! The change log gets autogenerated by the prep release GitHub workflow.

@steppi steppi added the enhancement New feature or request label Mar 29, 2024
@agriyakhetarpal agriyakhetarpal force-pushed the feat/add-default-global-button-text branch from b12996d to 244de38 Compare March 29, 2024 12:33
@steppi
Copy link
Collaborator

steppi commented Mar 29, 2024

Thanks @agriyakhetarpal! Looking good so far. One thing: I think that all of the global config options should be propagated if a custom option isn't given, not just try_examples_global_button text. You should also do try_examples_global_theme, and try_examples_global_warning_text.

@agriyakhetarpal
Copy link
Member Author

Right, I had thought of doing try_examples_global_warning_text too, but that would become a bigger breaking change, wouldn't it? People will have to override the global warning text and set it to None if they are using manual TryExamples directives somewhere. It does make sense, but I guess you're more aware of how downstream users would react, though.

I agree about try_examples_global_theme, however – I'll make that change and for the other global config options.

@steppi
Copy link
Collaborator

steppi commented Mar 29, 2024

Right, I had thought of doing try_examples_global_warning_text too, but that would become a bigger breaking change, wouldn't it? People will have to override the global warning text and set it to None if they are using manual TryExamples directives somewhere. It does make sense, but I guess you're more aware of how downstream users would react, though.

I agree about try_examples_global_theme, however – I'll make that change and for the other global config options.

I think making a breaking change here is fine. jupyterlite_sphinx hasn't hit a stable 1.0 yet and it even says in the docs that try_examples is an experimental directive:

`jupyterlite-sphinx` provides the experimental `try_examples` directive which allows
docstring examples sections written in [doctest format](https://docs.python.org/3/library/doctest.html) to be swapped with an embedded classic Notebook at the push of a button.

It would be surprising to me if some global config options were respected for manually inserted directives but not others. Since we're working with an experimental directive, we can make breaking changes to ensure consistent behavior. I think the default for global warning text is that there should be none. If someone sets a global warning text, and wants to override it in a few places, then I don't think it's a big deal for them to have to manually set the warning text to None for those specific instances.

@agriyakhetarpal
Copy link
Member Author

Fair point, didn't consider that the directive was/is still experimental – I have added in the requested changes! Now, one should be able to use the global warning text and use the global name for the theme/CSS container styling.

@agriyakhetarpal agriyakhetarpal changed the title Use try_examples_global_button_text if provided for TryExamples directive Allow usage of global configuration values for TryExamples directive if provided by user Mar 29, 2024
Copy link
Collaborator

@steppi steppi left a comment

Choose a reason for hiding this comment

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

Thanks @agriyakhetarpal. This is looking better. A few minor suggestions.

agriyakhetarpal and others added 2 commits March 30, 2024 20:24
Do not include a warning text by default, but if a
value exists in conf.py, use it. If a directive has its
own warning text, then override the global value.

Co-Authored-By: Albert Steppi <[email protected]>
Revert "Update options docs for `TryExamples` directive"

This reverts commit 40d4a72.

Revert "State default `:button_text` value if not provided"

This reverts commit 244de38.
Copy link
Collaborator

@steppi steppi left a comment

Choose a reason for hiding this comment

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

Thanks @agriyakhetarpal. this is in good shape now. Just a couple minor suggestions on language.

Copy link
Collaborator

@steppi steppi left a comment

Choose a reason for hiding this comment

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

Thanks @agriyakhetarpal! This looks good now.

@steppi steppi merged commit 86bdf60 into jupyterlite:main Mar 30, 2024
@agriyakhetarpal agriyakhetarpal deleted the feat/add-default-global-button-text branch March 30, 2024 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

try_examples_global_button_text does not propagate to custom .. try_examples:: directives
2 participants