Don't ignore first argument passed to sphinx.apidoc.main by adamjstewart · Pull Request #3668 · sphinx-doc/sphinx

adamjstewart · 2017-04-25T18:18:42Z

Feature or Bugfix

Bugfix? Depends on whether you consider it a bug or a feature 😉

Purpose

We use Sphinx to generate our documentation. Our API changes fairly often, so in order to make sure our API docs stay up-to-date, we always re-run sphinx-apidoc with each build. Unfortunately, Read the Docs skips our Makefile and directly calls:

$ sphinx-build -b html . _build/html

To get around this, we run sphinx-apidoc from our conf.py. Our call looks something like:

import sphinx.apidoc.main as sphinx_apidoc

sphinx_apidoc(['--force', '--no-toc', '--output-dir=.', '../spack'])

At first I was really confused why this gave different results than:

$ sphinx-apidoc --force --not-toc --output-dir=. ../spack

but after reading the source code, I quickly discovered that main ignores the first argument it is passed. This makes perfect sense when running sphinx-apidoc from the command line, as the first argument will be the name of the script. But it is much less intuitive when being called directly from main. This PR fixes that.

Detail

By default, main will still process sys.argv[1:], but if passed a list of arguments, it will now process all of them. I'm not sure if this bug (feature?) is documented already. My only concern is that this change isn't really backwards compatible. If you wanted it to be, I could theoretically check if the first argument is sphinx-apidoc and skip it if so.

Relates

First reported here:
https://github.com/LLNL/spack/pull/3982/files#r113266150

tk0miya · 2017-04-26T16:42:43Z

@shimizukawa could you review this? I don't know what is the standards in python.

BTW, sphinx-build, sphinx-quickstart and sphinx-autogen are also uses parser.parse_args(argv[1:]). So we should also fix them if you'll accept this.

$ grep -r parse_args sphinx
sphinx/apidoc.py:    (opts, args) = parser.parse_args(argv[1:])
sphinx/cmdline.py:        opts, args = parser.parse_args(list(argv[1:]))
sphinx/ext/autosummary/generate.py:    options, args = p.parse_args(argv[1:])
sphinx/quickstart.py:        opts, args = parser.parse_args(argv[1:])

adamjstewart · 2017-04-26T16:43:48Z

Let me know if this seems like the right thing to do. If so, I can modify the others as well.

tk0miya · 2017-04-26T16:47:13Z

either is fine to me. I don't have strong opinion.

shimizukawa

Ether OK to me.

tk0miya

Thank you for comment.
As I said, I'm okay to merge this after all other scripts are updated.

adamjstewart · 2017-05-07T12:59:35Z

Sounds good, I'll try to take a stab at this sometime this week.

adamjstewart · 2017-05-09T19:45:18Z

I think I got everything now? Hopefully you have good unit tests.

tk0miya

It seems test_extensions in test_quickstart.py failed.
https://travis-ci.org/sphinx-doc/sphinx/jobs/230493513#L1859

Could you check this please?

tk0miya · 2017-05-11T09:43:50Z

utils/check_sources.py


 if __name__ == '__main__':
-    sys.exit(main(sys.argv))
+    sys.exit(main(sys.argv[1:]))


You also need to update the main() in this file.
This causes following error:
https://travis-ci.org/sphinx-doc/sphinx/jobs/230493517#L746

tk0miya · 2017-05-13T07:59:18Z

sphinx/cmdline.py



-def main(argv):
+def main(argv=sys.argv[1:]):


mypy warns here: https://travis-ci.org/sphinx-doc/sphinx/jobs/231257064#L753
Please mark this line as # typing: ignore.

Thanks, I've never used mypy before. Alternatively, I could remove the default argument and go back to the way things were before. I was just trying to make everything the same.

Oh, sorry. I was wrong. # type: ignore is correct, not typing.

tk0miya · 2017-05-13T16:11:58Z

Merged. Thank you for contribution!

Sphinx 1.7.0 changed the behaviour of the internal sphinx "main" functions: sphinx-doc/sphinx#3668 In order to support both, we now need to manually strip argv[0] before calling sphinx.main(), and also provide a dummy first argument for pre-1.7 sphinx to skip automatically. We do this by effectively duplicating our first "real" argument (-b html) as -bhtml.

Sphinx 1.7.0 changed the behaviour of the internal sphinx "main" functions: sphinx-doc/sphinx#3668 In order to support both, we now need to manually strip argv[0] before calling sphinx.main(), and also provide a dummy first argument for pre-1.7 sphinx to skip automatically. We do this by effectively duplicating our first "real" argument (-b html) as -bhtml. (cherry picked from commit 13e3ccb)

adamjstewart added 2 commits April 25, 2017 13:04

Don't ignore first argument to sphinx.apidoc.main

3e968bb

Fix tests

6741c6a

tk0miya requested a review from shimizukawa April 26, 2017 16:38

tk0miya added the api:cmdline label Apr 26, 2017

tk0miya added this to the 1.7 milestone Apr 26, 2017

shimizukawa approved these changes May 7, 2017

View reviewed changes

tk0miya requested changes May 7, 2017

View reviewed changes

adamjstewart added 2 commits May 9, 2017 14:37

Update other commands as well

e989ed7

Update more files that hardcode sys.argv

392e6ec

tk0miya requested changes May 11, 2017

View reviewed changes

Fix failing unit tests

326ddd1

tk0miya reviewed May 13, 2017

View reviewed changes

adamjstewart added 2 commits May 13, 2017 10:06

Placate mypy

e2f49cf

typing -> type

8737bec

tk0miya merged commit 7cca0b4 into sphinx-doc:master May 13, 2017

adamjstewart deleted the fixes/apidoc-main-argv branch May 13, 2017 16:12

tk0miya added a commit that referenced this pull request May 13, 2017

Update CHANGES for PR #3668

5a83608

adamjstewart mentioned this pull request May 13, 2017

Sphinx no longer ignores first argument spack/spack#4243

Merged

astrofrog mentioned this pull request May 24, 2017

Test against developer version of Sphinx astropy/sphinx-automodapi#23

Merged

This was referenced Feb 14, 2018

sphinx_build with 1.7.0 sphinx-contrib/paverutils#1

Closed

sphinx-build dies under 1.7.0 #4615

Closed

ojwb mentioned this pull request Feb 19, 2018

Add flag to start snippets at first non-whitespace character. xapian/xapian#180

Closed

jaylett mentioned this pull request Feb 19, 2018

Support sphinx >= 1.7 while still working with < 1.7 xapian/xapian#188

Merged

This was referenced Jun 18, 2018

sphinx-build: error: argument -d/--maxdepth: invalid int value #5104

Closed

Fix #5104: apidoc: Interface of sphinx.apidoc:main() has changed #5106

Merged

adamjstewart mentioned this pull request Jul 31, 2019

Fix Sphinx deprecation warning when building docs spack/spack#12205

Merged

snyk-bot mentioned this pull request Aug 18, 2021

[Snyk] Upgrade karma from 4.2.0 to 6.3.4 MaxMood96/sphinx#1

Open

github-actions bot locked as resolved and limited conversation to collaborators Sep 13, 2021



		def main(argv):
		def main(argv=sys.argv[1:]):

Uh oh!

Conversation

adamjstewart commented Apr 25, 2017

Feature or Bugfix

Purpose

Detail

Relates

Uh oh!

tk0miya commented Apr 26, 2017

Uh oh!

adamjstewart commented Apr 26, 2017

Uh oh!

tk0miya commented Apr 26, 2017

Uh oh!

shimizukawa left a comment

Choose a reason for hiding this comment

Uh oh!

tk0miya left a comment

Choose a reason for hiding this comment

Uh oh!

adamjstewart commented May 7, 2017

Uh oh!

adamjstewart commented May 9, 2017

Uh oh!

tk0miya left a comment

Choose a reason for hiding this comment

Uh oh!

tk0miya May 11, 2017

Choose a reason for hiding this comment

Uh oh!

tk0miya May 13, 2017

Choose a reason for hiding this comment

Uh oh!

adamjstewart May 13, 2017

Choose a reason for hiding this comment

Uh oh!

tk0miya May 13, 2017

Choose a reason for hiding this comment

Uh oh!

tk0miya commented May 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants