-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Expand documentation for -M flag. #12685
base: master
Are you sure you want to change the base?
Changes from all commits
8574810
2a37332
01bdcf7
4c23fe1
acd5d41
780b4db
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 |
---|---|---|
|
@@ -9,10 +9,11 @@ | |
import multiprocessing | ||
import os | ||
import pdb # NoQA: T100 | ||
import re | ||
import sys | ||
import traceback | ||
from os import path | ||
from typing import TYPE_CHECKING, Any, TextIO | ||
from typing import IO, TYPE_CHECKING, Any, TextIO | ||
|
||
from docutils.utils import SystemMessage | ||
|
||
|
@@ -111,9 +112,55 @@ def jobs_argument(value: str) -> int: | |
return jobs | ||
|
||
|
||
class ParagraphFormatter(argparse.HelpFormatter): | ||
"""Wraps help text as a default formatter but keeps paragraps separated.""" | ||
|
||
_paragraph_matcher = re.compile(r"\n\n+") | ||
|
||
def _fill_text(self, text: str, width: int, indent: str) -> str: | ||
import textwrap | ||
result: list[str] = [] | ||
for p in self._paragraph_matcher.split(text): | ||
p = self._whitespace_matcher.sub(' ', p).strip() | ||
p = textwrap.fill(p, width, | ||
initial_indent=indent, | ||
subsequent_indent=indent) | ||
result.append(p) | ||
return '\n\n'.join(result) | ||
|
||
|
||
class ArgParser(argparse.ArgumentParser): | ||
"""Wraps standard ArgumentParser to add sphinx-specefic flags to help.""" | ||
|
||
def __inject_help_entry(self) -> argparse._ArgumentGroup: | ||
from gettext import gettext as _ | ||
# we inject -M flag action to positionals before printing help | ||
# so that there is no risk of side effects on actual execution | ||
for action_group in self._action_groups: | ||
if action_group.title != _('positional arguments'): | ||
continue | ||
m_flag = argparse.Action( | ||
["-M"], "BUILDER", # ugly but works | ||
help=__('please refer to usage and main help section') | ||
) | ||
action_group._group_actions.insert(0, m_flag) | ||
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. I don't like this. The If you want to keep this hack, restore the attributes that were mutated after printing as well. 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. I would like to push back on this specific comment. 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. Mmh. Let's live with this hack then. We prefer not to have force pushes since it's hard to review commits. We always squash merge at the end so you just push commits one by one. I'd suggest rebasing locally if you want to squash your own commits but avoid force-pushing in general. |
||
return action_group | ||
err = ( | ||
"Couldn't find the argument group 'positional arguments' to inject " | ||
"-M flag's help info into.") | ||
raise TypeError(err) | ||
|
||
def print_help(self, file: IO[str] | None = None) -> None: | ||
action_group = self.__inject_help_entry() | ||
super().print_help(file) | ||
del action_group._group_actions[0] | ||
|
||
|
||
def get_parser() -> argparse.ArgumentParser: | ||
parser = argparse.ArgumentParser( | ||
usage='%(prog)s [OPTIONS] SOURCEDIR OUTPUTDIR [FILENAMES...]', | ||
parser = ArgParser( | ||
usage=('%(prog)s [OPTIONS] SOURCEDIR OUTPUTDIR [FILENAMES...]\n ' | ||
'%(prog)s -M BUILDER SOURCEDIR OUTPUTDIR [OPTIONS]'), | ||
formatter_class=ParagraphFormatter, | ||
epilog=__('For more information, visit <https://www.sphinx-doc.org/>.'), | ||
description=__(""" | ||
Generate documentation from source files. | ||
|
@@ -123,6 +170,11 @@ def get_parser() -> argparse.ArgumentParser: | |
settings. The 'sphinx-quickstart' tool may be used to generate template files, | ||
including 'conf.py' | ||
|
||
Use the '-M' option to simultaneously build several formats into the same | ||
OUTPUTDIR. When specified, this option MUST be the first command-line argument. | ||
If a finer control over the output is needed, use the '-b/--builder' option | ||
instead. Note that these modes have a different signature. | ||
|
||
sphinx-build can create documentation in different formats. A format is | ||
selected by specifying the builder name on the command line; it defaults to | ||
HTML. Builders can also perform other tasks related to documentation | ||
|
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.
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.
@AA-Turner unfortunately the last suggestion is misleading. The whole point of this PR is to show that signatures differ. if we have an -M flag then options go AFTER positionals. If there is no -M then they go BEFORE positionals. This is precisely the reason why current docs are confusing and why there is a bunch of questions on stackoverflow addressing "broken" optionals.
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 is this? Options can go after positionals with no
-M
. See #12795 to add 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.
Ok, fair point,
-b
mode does not care that much about optionals' position because it delegates parsing toargparse
. But the main point is when we use-M
we MUST provide optionals after positionals and so far it is never stated in a clear form anywhere. Which is an issue in my opinion, at least for people who think like me and tend to stack optionals together.An examle to illustrate the issue:
sphinx-build -M html -v -n sphinx build/html
will lead torun_make_mode()
getting['-b', 'html', '-d', '-n\\doctrees', '-v', '-n\\html', 'sphinx', 'build/html']
. This happens becauseMake()
expects src and dst to immediately follow the-M
flag.This leads to a lot of frustration on part of anyone who just starts working with the tool. In my case it was
sphinx-build -M html -d build/doctree sphinx build/html
which is even whorse because it would not fail but put doctree into abuild/doctree/html
dir. So each specific build would rebuild doctrees. It took me 3 hours of debugging to figure out the issue. And how many people won't even try to pinpoint the issue?So my argument is this should be documented. Either the fact that
-M
requires positionals to immediately follow it or to present both signatures as different usecases. To be fair I would actually prefer to:-M
case processing toargparse
and make-M|b
mutually excluseive.The issue though is it would be a breaking change and as much as I like the idea, I don't want pipelines to crash. So I opted for an explicit documentation that would cover both official docs and an
-h
flag so no matter what you choose, you get that info and don't have to waste 3 hours figuring out why the tool does not work as expected.Now, getting back to your suggestion: It would work, yes. But it still would not make people treat
-M
and-b
as very different and mutually exclusive flags as they are. So I still prefer 2 signatures because it is explicit and leaves a lot less room for an error. Yes, the-b
signature might have some adjustments to address the flexibility of optionals' positioning, but IMO it still should be clearly stated that-M
and-b
are 2 VERY different things and should be treated accordingly. And this should not come in a form of a note hidden somewhere far down.I personally still have issues getting direct links to an official sphinx-documentation in my search results. I literally had to open the source code, find a couple key phrases and search by those to find the docs page in question. So I would prefer this info being present very close to any signature we have, be it the docs or an
-h
output.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.
@AA-Turner any feedback on the issue please?