Skip to content
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

docbuild: switch from optparse to argparse #31366

Closed
jhpalmieri opened this issue Feb 9, 2021 · 53 comments
Closed

docbuild: switch from optparse to argparse #31366

jhpalmieri opened this issue Feb 9, 2021 · 53 comments

Comments

@jhpalmieri
Copy link
Member

Sage's docbuilding uses optparse for argument parsing, but this library has been deprecated for a while. We should switch to argparse.

Some instructions for conversions can be found here: https://docs.python.org/3/library/argparse.html#help

Depends on #32946

Component: documentation

Author: Frédéric Chapoton

Branch/Commit: 0b62c16

Reviewer: John Palmieri

Issue created by migration from https://trac.sagemath.org/ticket/31366

@jhpalmieri jhpalmieri added this to the sage-9.3 milestone Feb 9, 2021
@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 13, 2021

comment:1

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

@mkoeppe mkoeppe modified the milestones: sage-9.3, sage-9.4 Feb 13, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.4, sage-9.5 Jul 19, 2021
@fchapoton
Copy link
Contributor

comment:3

a little bit in #32331

@fchapoton
Copy link
Contributor

Branch: public/ticket/31366

@fchapoton
Copy link
Contributor

Commit: 159ec4b

@fchapoton
Copy link
Contributor

comment:4

first tentative


New commits:

159ec4bfirst step towards using argparse for doc command line arguments

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 4, 2021

Branch pushed to git repo; I updated commit sha1. New commits:

56ce7b6more work on argparse for doc command line

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 4, 2021

Changed commit from 159ec4b to 56ce7b6

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 21, 2021

Changed commit from 56ce7b6 to 98824cd

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 21, 2021

Branch pushed to git repo; I updated commit sha1. New commits:

98824cdMerge branch 'public/ticket/31366' in 9.5.b1

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 29, 2021

Branch pushed to git repo; I updated commit sha1. New commits:

d9cd177Merge branch 'public/ticket/31366' in 9.5.b5
ec995b7some details

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 29, 2021

Changed commit from 98824cd to ec995b7

@fchapoton
Copy link
Contributor

Author: Frédéric Chapoton

@fchapoton
Copy link
Contributor

comment:8

not quite sure that it works perfectly, but it seems so

@fchapoton
Copy link
Contributor

comment:9

needs work

[dochtml]   File "/home/sagemath/sage-9.1/local/lib/python3.7/site-packages/sage_docbuild/__init__.py", line 1558, in setup_parser
[dochtml]     description=help_description(compact=True))
[dochtml] TypeError: __init__() got an unexpected keyword argument 'add_help_option'

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 30, 2021

Changed commit from ec995b7 to 3a4c572

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 30, 2021

Branch pushed to git repo; I updated commit sha1. New commits:

3a4c572more work on argparse for docbuild

@fchapoton

This comment has been minimized.

@jhpalmieri
Copy link
Member Author

comment:12

I'm getting the message AttributeError: module 'argparse' has no attribute 'OptionGroup'. I can't tell what the right replacement is — add_argument_group? add_subparser?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 31, 2021

Branch pushed to git repo; I updated commit sha1. New commits:

70b6aa6another fix for argparse in docbuild

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 1, 2021

Changed commit from 84bde61 to 8c8fc39

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 1, 2021

Branch pushed to git repo; I updated commit sha1. New commits:

8c8fc39still working on argparse for docbuilding

@fchapoton
Copy link
Contributor

comment:23

more progress done.

@fchapoton
Copy link
Contributor

comment:24

and the bot+linter are now green ; still needs tests and double-check, for sure

@jhpalmieri
Copy link
Member Author

comment:25

Looks very close now, docs build and tests pass.

The old version exited gracefully with the help message if you ran ./sage --docbuild, whereas this one prints an error message instead.

Another difference. Old:

% ./sage --docbuild -h
Usage: sage --docbuild [OPTIONS] DOCUMENT (FORMAT | COMMAND)

Build or return information about Sage documentation.

    DOCUMENT    name of the document to build
    FORMAT      document output format
    COMMAND     document-specific command

Note that DOCUMENT may have the form 'file=/path/to/FILE',
which builds the documentation for the specified file.

A DOCUMENT and either a FORMAT or a COMMAND are required,
unless a list of one or more of these is requested.

OPTIONS:
  Standard:

New:

% ./sage --docbuild -h
usage: sage --docbuild [OPTIONS] DOCUMENT (FORMAT | COMMAND)

Build or return information about Sage documentation. DOCUMENT name of the document to
build FORMAT document output format COMMAND document-specific command Note that DOCUMENT
may have the form 'file=/path/to/FILE', which builds the documentation for the specified
file. A DOCUMENT and either a FORMAT or a COMMAND are required, unless a list of one or
more of these is requested.

positional arguments:
  document
  {html,pdf,changes,htmlhelp,inventory,json,latex,linkcheck,pickle,web}

Standard:

Any idea why it's not preserving the line breaks in the help message? We could try the following changes, but I would like to understand why it's not respecting our formatting for the strings.

diff --git a/src/sage_docbuild/__init__.py b/src/sage_docbuild/__init__.py
index 1f692f1abe..84a6444ec1 100644
--- a/src/sage_docbuild/__init__.py
+++ b/src/sage_docbuild/__init__.py
@@ -1385,9 +1385,6 @@ def help_description(s="", compact=False):
     character.
     """
     s += "Build or return information about Sage documentation.\n\n"
-    s += "    DOCUMENT    name of the document to build\n"
-    s += "    FORMAT      document output format\n"
-    s += "    COMMAND     document-specific command\n\n"
     s += "Note that DOCUMENT may have the form 'file=/path/to/FILE',\n"
     s += "which builds the documentation for the specified file.\n\n"
     s += "A DOCUMENT and either a FORMAT or a COMMAND are required,\n"
@@ -1625,8 +1622,10 @@ def setup_parser():
                           help="if ARG is 'reference', list all subdocuments"
                           " of en/reference. If ARG is 'all', list all main"
                           " documents")
-    parser.add_argument("document", nargs='?', type=str)
-    parser.add_argument("format", nargs='?', type=str, choices=get_formats())
+    parser.add_argument("document", nargs='?', type=str,
+                        help='name of the document to build')
+    parser.add_argument("format", nargs='?', type=str, choices=get_formats(),
+                        help='document output format')
     return parser
 
 
@@ -1722,7 +1721,8 @@ def main():
     # trying to build.
     name, typ = args.document, args.format
     if not name or not type:
-        raise ValueError('both document and format should be given')
+        parser.print_help()
+        sys.exit(1)
 
     # Set up module-wide logging.
     setup_logger(args.verbose, args.color)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 5, 2021

Branch pushed to git repo; I updated commit sha1. New commits:

00d94fffix for empty input

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 5, 2021

Changed commit from 8c8fc39 to 00d94ff

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 5, 2021

Branch pushed to git repo; I updated commit sha1. New commits:

b02c912fine tuning details in argparse for docbuild

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 5, 2021

Changed commit from 00d94ff to b02c912

@fchapoton
Copy link
Contributor

comment:28

here is a new tentative

@jhpalmieri
Copy link
Member Author

comment:29

Looks good. I have a few more proposed changes: two of the "examples" in help_examples don't work, so I am fixing one and changing one. Also, ./sage --docbuild -H used to print a longer message, and I am restoring that. Let me know if you object to any of these changes, of course.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 6, 2021

Branch pushed to git repo; I updated commit sha1. New commits:

c6e7bdatrac 31366: minor fixes to help_examples

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 6, 2021

Changed commit from b02c912 to c6e7bda

@jhpalmieri
Copy link
Member Author

comment:31

A bit more explanation: ./sage -FDC all used to print formats, documents, and commands, but now it just prints formats and exits. Also, the -S option requires -S=-OPTS because of the hyphen in OPTS; this appears to be a known issue with argparse: https://stackoverflow.com/questions/16174992/cant-get-argparse-to-read-quoted-string-with-dashes-in-it. Finally, ./sage --docbuild developer -j html is not good: the document and format should come first, the -j just confuses things (and isn't necessary anyway, since mathjax has been the default forever).

@jhpalmieri
Copy link
Member Author

comment:32

Ready for review? I am happy with your changes.

@jhpalmieri
Copy link
Member Author

Reviewer: John Palmieri

@fchapoton
Copy link
Contributor

comment:34

I would say so. I have forgotten if there were remaining issues or not.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 29, 2021

Branch pushed to git repo; I updated commit sha1. New commits:

ef2b25bRemove handling of broken option SAGE_DOC_MATHJAX=no
0b62c16Merge branch 't/32946/_sage___docbuild_doc_html__is_broken' into t/31366/public/ticket/31366

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 29, 2021

Changed commit from c6e7bda to 0b62c16

@jhpalmieri
Copy link
Member Author

Dependencies: #32946

@jhpalmieri
Copy link
Member Author

comment:36

Okay, let's merge it. (I added #32946 as a dependency because otherwise there would be a merge conflict.)

@vbraun
Copy link
Member

vbraun commented Dec 12, 2021

Changed branch from public/ticket/31366 to 0b62c16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants