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

process cell tags remove-input and remove-output #543

Closed
wants to merge 3 commits into from

Conversation

t-vi
Copy link

@t-vi t-vi commented Mar 2, 2021

This PR adds removing cell input and output by using the remove-input and remove-output tags.

My understanding from #436 is that the behaviour (remove output rather than hide with css) is consistent with Jupyter Book use of the same tags.
This is complimentary to #436 in that it deals with remove- tags as opposed to hide-*.

Thank you for looking at this, and making nbsphinx in the first place!

@t-vi
Copy link
Author

t-vi commented Mar 2, 2021

@mgeier thank you for making nbsphinx it is just what I need to keep a large part of my documentation in notebooks.

So my use-case is that I have (graphviz) graphs in the documentation that I want to generate from the notebook but I don't want to include the source code in the HTML because it adds nothing in my case (and people can always jump to the notebook if they want).

I hope this can fill a gap, but if this is totally different from what you would have in mind for handling these tags, don't hesitate to reject the PR, too.

@mgeier
Copy link
Member

mgeier commented Mar 5, 2021

Thanks for this PR!

I'm open for this feature, but I think there are some details still to be ironed out.

Can you please remove the outputs from the example notebook?

In your example, the padding after an input cell without output is missing.
So is the padding before an output cell without input.

The implementation is fine, but did you think about re-using the tag-based filtering from nbconvert?
Would this be possible?

I think remove-input and remove-output are good defaults, but what about allowing configuration options for specifying other tag names (like IIRC it is possible in nbconvert)?

My understanding from #434 is that the behaviour (remove output rather than hide with css) is consistent with Jupyter Book use of the same tags.

Is there a link to some documentation?

This is complimentary to #434

I don't see the connection.
Do you happen to mean #436?
Or #131?

BTW, did you have a look at #185 and #86?

@t-vi
Copy link
Author

t-vi commented Mar 8, 2021

I don't see the connection.
Do you happen to mean #436?

Oh sorry. Yes, I changed the text to #436.

BTW, did you have a look at #185 and #86?

I didn't. So I am aware of hide_input but ended up implementing remove-cell as a tag (by adding or "remove-cell" not in ...tags) in my local branch (which I'd happily add here, too). I must admit that tags are about my limit w.r.t. discoverability (it's bad, but it's the best we have, and standardizing across multiple implementations might be best, so I took the JupyterBook tags based on my impression that it seemed reasonable in #436) the custom metadata is more a way to sell headache pills to me than a user interface (and this is not to take a dig at nbsphinx using them when they were the best thing we had, but just that I had a bad time with them when using them for global things and I think they're best avoided for per-cell things due to the complexity of the UI).

Can you please remove the outputs from the example notebook?
Will do.

In your example, the padding after an input cell without output is missing.
So is the padding before an output cell without input.

The implementation is fine, but did you think about re-using the tag-based filtering from nbconvert?
Would this be possible?

Edit: Actually this works! I'll update the PR.

P.S.: Our project using nbsphinx and also your insipid theme went public today. Thank you for covering us for our documentation.

@mgeier
Copy link
Member

mgeier commented Mar 9, 2021

Thanks for the update!

the custom metadata is more a way to sell headache pills to me than a user interface (and this is not to take a dig at nbsphinx using them when they were the best thing we had, but just that I had a bad time with them when using them for global things and I think they're best avoided for per-cell things due to the complexity of the UI).

I agree.
I think I've already added support for tags for all features which were originally using custom cell metadata. If I've missed something, please let me know!

In fact, I would change the documentation to mention the tags first, and then in the end mention the custom cell metadata just as a legacy feature that still works for compatibility but is not recommended for new notebooks.
But we can also do this in a separate PR, if you prefer.


Can you please answer my remaining questions from #543 (comment)?


P.S.: Our project using nbsphinx and also your insipid theme went public today. Thank you for covering us for our documentation.

That's great to hear!

If you have suggestions for improving the theme, please let me know!

@t-vi
Copy link
Author

t-vi commented Mar 9, 2021

So I think the main ones are

I think remove-input and remove-output are good defaults, but what about allowing configuration options for specifying other tag
names (like IIRC it is possible in nbconvert)?

Personally, I don't really see the value. Jupyter seems to be unable to specify common tags, but in the end I see more value with having some sort of standardization and so aligning with Jupyter Book + Hardcoding might be a feature. That said, if you intended the question to express a preference for configuration, I'll add that.

My understanding from #434 is that the behaviour (remove output rather than hide with css) is consistent with Jupyter Book use of the same tags.

Is there a link to some documentation?

https://jupyterbook.org/interactive/hiding.html#removing-code-cell-content

Would you add that link to the documentation? I can do that when I flip the order of the hide methods in the documentation.

I think I have the other questions, but I'd appreciate a hint if not.

@t-vi
Copy link
Author

t-vi commented Mar 9, 2021

If you have suggestions for improving the theme, please let me know!

The most annoying thing was that Jupyter Notebooks don't need a blank line but nbsphinx generates funny stuff without (seems to be jupyter/nbconvert#917, so not a nbsphinx bug).

Other than that I probably haven't used half the customization that is possible. One thing I thought about is whether to put the logo in the topbar instead of the sidebar. That likely is already possible but I took your advice and focus on the content. I really like how nbsphinx and your insipid theme made that possible!

@mgeier
Copy link
Member

mgeier commented Mar 15, 2021

I think remove-input and remove-output are good defaults, but what about allowing configuration options for specifying other tag
names (like IIRC it is possible in nbconvert)?

Personally, I don't really see the value.

I don't have a concrete use case myself, but I could imagine that somebody wants different versions of their docs, once with removed cells and another one where all cells are visible.
Or they are already using a set of tags for something else, and suddenly they want to hide all those tagged cells.

If you look at it from a cost/benefit perspective: It costs basically nothing to expose the configuration options, therefore it doesn't matter if the benefit isn't huge.

Having the tags as literal strings littered around the source code is bad anyway, so at least I would make global variables for them. But making them Sphinx configuration options is just as easy, and to me it is quite an obvious thing to do (even if I don't have a use for that myself).

Jupyter seems to be unable to specify common tags,

Yeah, that's even more reason to make them configurable in nbsphinx!

but in the end I see more value with having some sort of standardization and so aligning with Jupyter Book + Hardcoding might be a feature.

Sure, standardization would be great, but as you said, this didn't happen (except for metadata.jupyter.source_hidden and metadata.jupyter.outputs_hidden: https://nbformat.readthedocs.io/en/latest/format_description.html#cell-metadata).

Jupyter Book is just another third-party package, it isn't in the position to standardize anything.

But if there are no better suggestions, I think it's reasonable to use their settings as our defaults.

That said, if you intended the question to express a preference for configuration, I'll add that.

Yeah, I guess I did.
Yes, please!

My understanding from #434 is that the behaviour (remove output rather than hide with css) is consistent with Jupyter Book use of the same tags.

Is there a link to some documentation?

https://jupyterbook.org/interactive/hiding.html#removing-code-cell-content

Would you add that link to the documentation? I can do that when I flip the order of the hide methods in the documentation.

Thanks for the link.
I think that adds no value to our documentation. We have to mention the default tag names anyway.

I think I have the other questions, but I'd appreciate a hint if not.

I think those were indeed all the questions, but I think this comment hasn't been addressed yet:

"In your example, the padding after an input cell without output is missing.
So is the padding before an output cell without input."


If you have suggestions for improving the theme, please let me know!

The most annoying thing was that Jupyter Notebooks don't need a blank line but nbsphinx generates funny stuff without (seems to be jupyter/nbconvert#917, so not a nbsphinx bug).

Yeah, the problem is that there are different Markdown implementation with slightly different behavior.
For now, nbsphinx is using pandoc (which itself supports multiple Markdown dialects), but in the future I'd like to use a Python library (which probably doesn't exist yet) for this.

Other than that I probably haven't used half the customization that is possible. One thing I thought about is whether to put the logo in the topbar instead of the sidebar.

There is no specific setting for putting the logo there, but you can try to use the left_buttons option for that.

But I think in general it's a waste of space to put the logo into the topbar.

You should think about users with small screens (like smartphones). Putting the logo there will make the topbar actually less useful for them.

You could of course add some CSS to avoid showing the logo on small screens, but if the logo is so important to you, you probably also want it to be shown on those small screens, right?

What about putting the logo at the top of each page (using the page.html template)?

That likely is already possible but I took your advice and focus on the content. I really like how nbsphinx and your insipid theme made that possible!

That's great to hear!

@mgeier
Copy link
Member

mgeier commented Mar 15, 2021

The PDF output seems to have the same problem, there is no space between cells: https://648-46379698-gh.circle-artifacts.com/0/nbsphinx.pdf#section.6

@t-vi
Copy link
Author

t-vi commented Mar 15, 2021

The PDF output seems to have the same problem, there is no space between cells: https://648-46379698-gh.circle-artifacts.com/0/nbsphinx.pdf#section.6

Yes, but so this happens when I use the nbconvert facility, so it would appear to be a bug there.

@mgeier
Copy link
Member

mgeier commented Mar 17, 2021

The PDF output seems to have the same problem, there is no space between cells: https://648-46379698-gh.circle-artifacts.com/0/nbsphinx.pdf#section.6

Yes, but so this happens when I use the nbconvert facility, so it would appear to be a bug there.

It very likely is a bug there, but nbsphinx doesn't use the LaTeX/PDF workflow that nbconvert uses (it uses the reST template which is later converted to LaTeX by Sphinx).

Either way, I would like it to work correctly in nbsphinx, but if it gets fixed in nbconvert as well, that's of course also good.

@t-vi
Copy link
Author

t-vi commented Mar 17, 2021

Either way, I would like it to work correctly in nbsphinx, but if it gets fixed in nbconvert as well, that's of course also good.

I'm afraid my notebook foo isn't enough to implement these tags after all.

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.

2 participants