Expand documentation for -M flag. #12685
Conversation
|
@AA-Turner any feedback please? |
doc/man/sphinx-build.rst
Outdated
| @@ -4,14 +4,18 @@ sphinx-build | |||
| Synopsis | |||
| -------- | |||
|
|
|||
| **sphinx-build** [*options*] <*sourcedir*> <*outputdir*> [*filenames* ...] | |||
| | **sphinx-build** -M <*builder*> <*sourcedir*> <*outputdir*> [*options*] | |||
There was a problem hiding this comment.
I'd prefer having the -M as an alternative. Without -M is much more common I think.
sphinx/cmd/make_mode.py
Outdated
| @@ -86,14 +86,14 @@ def build_clean(self) -> int: | |||
| return 0 | |||
|
|
|||
| def build_help(self) -> None: | |||
| if not color_terminal(): | |||
| if not color_terminal() or '--no-color' in sys.argv or '-N' in sys.argv: | |||
There was a problem hiding this comment.
Use self.opts instead of sys.argv. I think they should be equivalent.
sphinx/cmd/build.py
Outdated
| parser = argparse.ArgumentParser( | ||
| usage='%(prog)s [OPTIONS] SOURCEDIR OUTPUTDIR [FILENAMES...]', | ||
| parser = ArgParser( | ||
| usage=('%(prog)s -M BUILDER SOURCEDIR OUTPUTDIR [OPTIONS]\n ' |
There was a problem hiding this comment.
Put the Make mode as the second mode.
sphinx/cmd/build.py
Outdated
| PLEASE NOTE that there are 2 signatures for 2 usage scenarios. -M flag is a | ||
| special flag that must always come first if used! If present, it alters the | ||
| way the tool works. It can be thought of as a "one-click" solution. It allows | ||
| to build several formats of docs into the same OUTPUTDIR without mixing them | ||
| up (it creates a dedicated directory for each builder). If more control over | ||
| paths is needed then -b flag is the way to go. |
There was a problem hiding this comment.
| PLEASE NOTE that there are 2 signatures for 2 usage scenarios. -M flag is a | |
| special flag that must always come first if used! If present, it alters the | |
| way the tool works. It can be thought of as a "one-click" solution. It allows | |
| to build several formats of docs into the same OUTPUTDIR without mixing them | |
| up (it creates a dedicated directory for each builder). If more control over | |
| paths is needed then -b flag is the way to go. | |
| 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. |
| ["-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.
I don't like this. The argparse module is very fragile, so please find
an alternative instead of this hack (especially when it comes to groups).
If you want to keep this hack, restore the attributes that were mutated after printing as well.
There was a problem hiding this comment.
I would like to push back on this specific comment. argparse is what is used by the original code and there is no way to replace it without changing a whole lot of things. Since @AA-Turner seems to build a new version of a cli (at least it looks like that from his last commits), there isn't much sense to me to reinvent a second bicycle.
As to reverting back the attributes after the injection: it is performed specifically in a call of print_help. This call always ends with a termination of a program so there is no point in reverting the changes. It is exactly the reason I have implemented it in the call itself.
As for your other comments - thank you for your input, I will try to implement and push those as soon as I can. Is there a preferred way to sync branches for sphinx specifically? I prefer using rebase but it would cause your comments to break after force push which I'm not sure is a good thing.
There was a problem hiding this comment.
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.
doc/tutorial/getting-started.rst
Outdated
| Please note that ``-M`` flag is a *make-mode* flag. ``sphinx-build`` also has | ||
| a *build-mode* that provides more control over cache directories but has less | ||
| builders available and has a different signature. It is invoked by using a | ||
| ``-b`` flag. These flags are mutually exclusive. You can find out more in | ||
| :doc:`sphinx-build's manual </man/sphinx-build>`. |
There was a problem hiding this comment.
| Please note that ``-M`` flag is a *make-mode* flag. ``sphinx-build`` also has | |
| a *build-mode* that provides more control over cache directories but has less | |
| builders available and has a different signature. It is invoked by using a | |
| ``-b`` flag. These flags are mutually exclusive. You can find out more in | |
| :doc:`sphinx-build's manual </man/sphinx-build>`. | |
| :option:`sphinx-build -M` uses the *make-mode* but :program:`sphinx-build` | |
| also supports a *build-mode* via :option:`sphinx-build -b` to provide a | |
| finer control over the generated artifacts. | |
| See the :ref:`sphinx-build's manual </man/sphinx-build>` for details. |
doc/man/sphinx-build.rst
Outdated
| Please note that there are 2 signatures for 2 usage scenarios. ``-M`` flag | ||
| invokes a :ref:`make mode <make_mode>`, otherwise it works in a *build mode*. |
There was a problem hiding this comment.
| Please note that there are 2 signatures for 2 usage scenarios. ``-M`` flag | |
| invokes a :ref:`make mode <make_mode>`, otherwise it works in a *build mode*. | |
| The available modes are the *build-mode* via :option:`sphinx-build -b` | |
| and the *make-mode* via :option:`sphinx-build -M`. |
There was a problem hiding this comment.
I would also suggest adding at least a mention of the fact that signatures for those mods are different. This fact is obvious for long time users while often gets completely missed by new users and leads to hours of wasted time. A couple extra words in exchange for hours saved is IMO a good trade off.
There was a problem hiding this comment.
You can add this note at the end of sentence, e.g., "Note that these modes have a different signature. The make-mode always requires the -M argument to comes first."
|
@picnixz I've implemented your suggestions. I also decided to add a failsafe onto the help injection (you never know how people would hack into the tool) plus added an additional guard to bail if for some reason the section we need is absent (severe |
|
I'll review it either today or tomorrow (I've got a lot of PRs to review and some additional work to do...) |
| | **sphinx-build** [*options*] <*sourcedir*> <*outputdir*> [*filenames* ...] | ||
| | **sphinx-build** -M <*builder*> <*sourcedir*> <*outputdir*> [*options*] |
There was a problem hiding this comment.
| | **sphinx-build** [*options*] <*sourcedir*> <*outputdir*> [*filenames* ...] | |
| | **sphinx-build** -M <*builder*> <*sourcedir*> <*outputdir*> [*options*] | |
| **sphinx-build** [-M <*builder*>] <*sourcedir*> <*outputdir*> [*filenames* ...] [*options*] |
There was a problem hiding this comment.
@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.
If there is no -M then they go BEFORE positionals.
Why is this? Options can go after positionals with no -M. See #12795 to add tests.
There was a problem hiding this comment.
Why is this? Options can go after positionals with no
-M.
Ok, fair point, -b mode does not care that much about optionals' position because it delegates parsing to argparse. 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 to run_make_mode() getting ['-b', 'html', '-d', '-n\\doctrees', '-v', '-n\\html', 'sphinx', 'build/html']. This happens because Make() 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 a build/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:
- a) split them into 2 subcommands
- b) move
-Mcase processing toargparseand make-M|bmutually 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.
@AA-Turner any feedback on the issue please?
Expand existing documentation for
sphinx-builds-Mflag and it's peculiarities.Feature
Purpose
Current documentation for the
-Mflag is pretty sparse and hard to reach. Given the fact it is not set up as a mutually exclusive option with the-bflag yet they both expect different args order, this leads to a lot of confusion for newcomers. This PR aims to rectify this by expanding the docs for both cli's--helpoutput and documentation itself.As an added bonus it also addresses coloring issues for
-M helpoutput.Note
This PR needs a spelling check! I'm not a native speaker and tend to do mistakes or formulate sentences in a manner that fits english ear wrong.
Detail
Fixes that address the
-Mflag issues:-Mflag (as a bonus it now respects paragraphs in the description)Fixes that address the color issue:
helpbuilder now respects--no-colorflagsRelates
-Mflag related:Color related: