- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 684
HTML documentation: Show preparsed doctests using inline tabs #37083
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
Changes from all 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 | 
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| div.jupyter_container { | ||
| margin: .5rem 0; | ||
| border: 0; | ||
| } | ||
|  | ||
| div.jupyter_container + div.jupyter_container { | ||
|  | ||
| 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: