-
-
Notifications
You must be signed in to change notification settings - Fork 481
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
Changes from 13 commits
d318a49
10acc0f
8974bcb
129540c
6412ce4
161a7e7
0034cc9
d8b0a12
84e7c89
5c8bfa1
57fd4ab
abfda0a
75d0e7d
f74f017
dea37e7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -289,6 +289,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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. More rules for review
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Just a moment ago you denied that the switches even 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 commentThe 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 commentThe 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. |
||
action="store_true", | ||
help="do not show preparsed versions of EXAMPLES blocks") | ||
standard.add_argument("--include-tests-blocks", dest="skip_tests", default=True, | ||
action="store_false", | ||
help="include TESTS blocks in the reference manual") | ||
|
@@ -478,6 +481,8 @@ def excepthook(*exc_info): | |
build_options.ALLSPHINXOPTS += "-n " | ||
if args.no_plot: | ||
os.environ['SAGE_SKIP_PLOT_DIRECTIVE'] = 'yes' | ||
if args.no_preparsed_examples: | ||
os.environ['SAGE_PREPARSED_DOC'] = 'no' | ||
if args.live_doc: | ||
os.environ['SAGE_LIVE_DOC'] = 'yes' | ||
if args.skip_tests: | ||
|
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: