-
-
Notifications
You must be signed in to change notification settings - Fork 480
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
HTML documentation: Show preparsed doctests using inline tabs #37083
Conversation
…) using inline tabs
…E_PREPARSED_DOC (default yes)
…-preparsed-examples
@@ -67,7 +68,7 @@ for REPO in ${SAGE_CI_FIXES_FROM_REPOSITORIES:-sagemath/sage}; do | |||
git am --signoff --show-current-patch=diff | |||
echo "--------------------------------------------------------------------8<-----------------------------" | |||
echo "::endgroup::" | |||
echo "Failure applying $PULL_URL as a patch, resetting" | |||
echo "Failure applying $PULL_SHORT as a patch, resetting" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the full URL since it easy to open the corresponding PR. Either way, these changes are unrelated to the actual goal of the PR and thus should be moved to somewhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The full URL appears just above. The improvement is that the repeated one is shortened so that there is not a second, duplicate link.
I'll not open a separate PR for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not? This is clearly not connected to main theme of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've just finished the review of this one line change, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, and there are also more changes here that seem unrelated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You commented, I responded to your comment. What's missing for the review of this one line change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rules of review, spelled out for you:
- Provide the necessary information.
- Do not use vagueness as a tool.
- Do not ask for information that has already been provided.
build/pkgs/sphinx_inline_tabs/type
Outdated
@@ -0,0 +1 @@ | |||
standard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a new standard package needs a period where it is optional and then a vote on the mailing list first, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't do that for trivialities.
@@ -288,6 +288,9 @@ def setup_parser(): | |||
standard.add_argument("--no-plot", dest="no_plot", | |||
action="store_true", | |||
help="do not include graphics auto-generated using the '.. plot' markup") | |||
standard.add_argument("--no-preparsed-examples", dest="no_preparsed_examples", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the use case for this option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's faster and some people may not want to see the tabs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Speedwise it doesn't seem to make a change (by looking at the github workflow execution time). Thus, to reduce complexity I prefer to just remove the option here and always build the docs with the parsed python tab.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted that this is your preference. I won't remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you want to make this feature configurable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More rules for review
- Check the facts about an unfamiliar domain before making claims about it
Specifically here: Any developer who has worked with the Sage docbuild system knows that we have long had a range of options that configure global aspects of it. There's SAGE_SKIP_PLOT_DIRECTIVE
(which you see even in the diff of the present PR...), SAGE_DOC_UNDERSCORE
, SAGE_USE_CDNS
, and we used to have SAGE_DOC_MATHJAX
, SAGE_DOC_JSMATH
until recently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In each of these cases there is a good reason for why the switch exists beyond "some people may not want to" have xyz.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a moment ago you denied that the switches even exist.
Now you say they exist but they have a better reason to exist.
This is called "moving the goalposts", a well-known standard technique.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goalpost stayed the same: we should not add config options to toggle default documentation features off just because "some people may not want to see [xyz]".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're repeating yourself.
Just because you have this opinion does not mean that this PR needs work.
There are overlapped tabs like This is from Sage can handle very large numbers, even numbers with millions or billions of
digits.
::
sage: factorial(100)
93326215443944152681699238856266700490715968264381621468592963895217599993229915608941463976156518286253697920827223758251185210916864000000000000000000000000
::
sage: n = factorial(1000000) # about 1 second
sage: len(n.digits())
5565709
This computes at least 100 digits of :math:`\pi`. It would be better to separate tabs for the two blocks of examples. Is this possible? |
Thanks for finding this bug. I'll investigate |
I think I prefer "Sage (live)" for its clearer semantics ("same language (but a variant)") and extensibility (we can add "Python (live)" later). But I also do have to admit that I generally like parentheses, which is probably related to the amount of time I spent with Lisp in the past~ |
OK. This is just a matter of personal preference, where the author's preference takes precedence.
That is interesting, though I cannot imagine it now.
I don't like parentheses, as they add branches (obstructions) to the (linear) flow of sentences. I think I learned it from Knuth's advices on writing: one of the last things he does after writing a sentence is to remove unnecessary parentheses from it. |
SageMath version 10.3.beta8, Release Date: 2024-02-13
How to turn off tabs? Is it possible to turn off tabs with It seems that
turns on tabs. |
Yes, that's right. Would you prefer the tabless version for this case generally, or as an option? |
I am not sure... What is the use of the preparsed doc? I guess that primarily advanced users of distribution packages may find it useful. I can imagine that a beginner may become aware of the preparsing mechanism and see how sage syntax is different from python syntax, which is an essential knowledge for effective use of sage. Users in the middle ground may find tabs (with preparsed doc) just take spaces for information of little use. Having tabs for each example that synchronize anyway seems a waste of space. I feel that somehow we should have configuration option or switch on pages to support the users in the middle ground. And whatever option or switch we implement should get along with the configuration option and switch for live doc. I am not sure how we could do this without making things too complicated... |
Yes, all of that. But it also helps anyone who is already an experienced Python programmer and comes across Sage. It makes it obvious and immediately testable that Sage is actual Python and usable from Python as a library, not some kind of quasi-Python, Python-like, Python-inspired kind of specialty language, or Python with training wheels.
Yes, the vertical space is precious, which is exactly why I made it disablable.
A persistent switch (is the light/dark selector persistent? using cookies?) for users who only want to see one version and save the vertical space could be an interesting follow-up. But that needs to be done by someone other than me; I stopped following web technologies around 1997;) |
I think I can suggest these for now:
|
Nowadays, AI helps humans catch up :-) |
Why would that possibly be an improvement? |
Why would that be better? I much prefer to click the middle tab instead of having to chase down that shy "Make live" button that lives in hiding near the window frame |
I assumed all tabs are already in either "parsed" or "preparsed" by default as the user want. Then the user would want to switch to the other version temporarily for a particular tab. But right... in my proposal, it is not handy for the user to choose the the default, unless to make a full doc build. OK. I am not strong about this. |
By having the two features orthogonal, we don't need to add an additional tab for "live preparsed". But I agree that the button is not so visible... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK.
Now I am convinced that having live tab is better than inserting live cell below.
By the way, live doc seems broken (as seen in the doc preview). Perhaps temporarily. Thebe was upgraded to 0.9.0 recently. |
Are we not pinning the versions of the components there? |
We are using https://unpkg.com/thebe@latest/lib/index.js via patched jupyter-sphinx. |
Fixing in #37637 |
You may want these refinements: diff --git a/src/doc/common/static/custom-jupyter-sphinx.css b/src/doc/common/static/custom-jupyter-sphinx.css
index a68a5cb05aa..e79b47b4539 100644
--- a/src/doc/common/static/custom-jupyter-sphinx.css
+++ b/src/doc/common/static/custom-jupyter-sphinx.css
@@ -1,5 +1,5 @@
div.jupyter_container {
- margin: .5rem 0;
+ border: 0;
}
div.jupyter_container + div.jupyter_container {
diff --git a/src/sage_docbuild/conf.py b/src/sage_docbuild/conf.py
index 145105bb814..1c705c68d1f 100644
--- a/src/sage_docbuild/conf.py
+++ b/src/sage_docbuild/conf.py
@@ -903,7 +903,7 @@ class SagecodeTransform(SphinxTransform):
linenostart=1)
cell_node += cell_input
container = TabContainer("", type="tab", new_set=False)
- textnodes = [Text('Sage (live)')]
+ textnodes = [Text('Sage Live')]
label = Label("", "", *textnodes)
container += label
content = Container("", is_div=True, classes=["tab-content"]) "Sage Live" or "Sage live" (no parentheses please). |
Thank you, committed as dea37e7. |
Documentation preview for this PR (built with commit dea37e7; changes) is ready! 🎉 |
Every doctest block is decorated with tabs, by default showing the original doctest in Sage syntax, and offering a preparsed, pure Python version of it. The tabs are synchronized across the page.
If @kwankyu's live documentation is being built, it is offered as another tab Sage (live). When this tab is selected, it automatically starts the Thebe/Binder; it is not necessary to find and push the "Make live" button.
Preview
Fixes #35791
📝 Checklist
⌛ Dependencies
.. envvar::
to define environment variables #37065