Add block-level sphinx_gallery_capture_repr_block setting#1398
Add block-level sphinx_gallery_capture_repr_block setting#1398larsoner merged 7 commits intosphinx-gallery:masterfrom
sphinx_gallery_capture_repr_block setting#1398Conversation
|
Hey again @larsoner! It was pretty straightforward to implement what we discussed in #1397 👍 I opened this PR for you to take a look at and get some feedback first. Happy to work on documenting this in https://sphinx-gallery.github.io/stable/configuration.html once you're happy with the implementation |
larsoner
left a comment
There was a problem hiding this comment.
Awesome, does seem to look correct!
Can you update https://github.com/sphinx-gallery/sphinx-gallery/blob/master/doc/configuration.rst ?
Bonus points if you want to add a .. warning:: or .. admonition:: that mentions that options like this one make SG behave differently from how users will experience things in the notebook, and that they are generally discouraged because they makes for an inconsistent experience. There are a few options like this but if you can at least list a few of them, that would already be a good start!
…_repr_block` setting
|
@larsoner I did't find a clean nice way of introducing the Let me know what you think |
|
Maybe it's easier for you to review the final text like this:
and I ended up adding a note admonition but to connect the suggestions for matplotlib figures to Plotly figures:
|
doc/configuration.rst
Outdated
| of Sphinx-Gallery prior to v0.5.0. Note however that this behaviour is not | ||
| consistent with how Jupyter notebooks capture output by default! |
There was a problem hiding this comment.
I think it is not necessarily setting capture_repr to empty tuple in general that we want to warn about, I think it's more altering it for specific files or code blocks when you've set it to something else globally.
There was a problem hiding this comment.
Hey @lucyleeow thanks for the review. I think you and @larsoner obviously have much more experience with this framework and with how users interact with it so please feel free to push the changes you see fit 👍🏽
My 2 cents is that both warnings seem to be redundant. The users will only use these settings knowing what they do so it should be obvious that setting a different value at the block-level will result in different or inconsistent behaviour to other blocks that inherit the global setting. i.e., I don't really see what new information the user is learning by warning them that changing the setting at the block-level will result in a different behaviour when compared to other blocks.. that's the whole point of block-level settings 😄
But again, I don't have strong feelings about this, so feel free to push or let me know what you'd like to see here instead.
Thanks in advance!!
There was a problem hiding this comment.
The users will only use these settings knowing what they do so it should be obvious that setting a different value at the block-level will result in different or inconsistent behaviour to other blocks that inherit the global setting. i.e., I don't really see what new information the user is learning by warning them that changing the setting at the block-level will result in a different behaviour when compared to other blocks.. that's the whole point of block-level settings
@tpvasconcelos when thinking about this issue it's important to be clear about who "users" are. I think you are using the term to mean "consumers of sphinx-gallery, i.e., (largely) Python library authors/devs building a gallery". My warning has more to do with "end users of a given Python library who are browsing and using the Sphinx-gallery built doc". It's for these latter (end-)users where the inconsistency or surprising behavior can happen. Hopefully the warning I added makes this a bit clearer!
There was a problem hiding this comment.
@larsoner gotcha! Sounds reasonable to me 👍🏽
larsoner
left a comment
There was a problem hiding this comment.
LGTM @lucyleeow feel free to merge if you're happy
| .. admonition:: Diverging from Jupyter | ||
| :class: danger | ||
|
|
||
| Sphinx-gallery attempts to render examples to HTML in a manner largely consistent with what a user will experience when they download the corresponding ``.ipynb`` notebook file and run it locally. | ||
| Some options, such as ``'capture_repr': (),``, will make these behaviors less consistent. | ||
| Consider using these options sparingly as it could lead to confusion or sub-optimal experiences for users! |
There was a problem hiding this comment.
Pushed a commit with this text to cover what I had in mind! @lucyleeow feel free to look and make sure you think it's reasonable
| dg.edge(node_from, node_to, color=API_COLORS["edge"]) | ||
| # add modules with all API entries | ||
| for module in gallery_conf["_sg_api_entries"]["module"]: | ||
| for module in gallery_conf["_sg_api_entries"].get("module", []): |
There was a problem hiding this comment.
Every once in a while when rebuilding MNE-Python doc I'd get an error on this line. No idea why and couldn't figure out how to reproduce it, but it should be safe enough to use .get instead of direct dict access here.
|
Thanks @larsoner and @lucyleeow for the prompt feedback and fast turnaround on this issue! 🚀🚀 |
|
Pushed nitpick fix and marking for merge-when-green, thanks in advance @tpvasconcelos ! |
closes #1397