search

sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.43k stars 479 forks source link

docbuild: switch from optparse to argparse #31366

Closed jhpalmieri closed 2 years ago

jhpalmieri commented 3 years ago

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

mkoeppe commented 3 years ago
comment:1

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

fchapoton commented 3 years ago
comment:3

a little bit in #32331

fchapoton commented 3 years ago

Branch: public/ticket/31366

fchapoton commented 3 years ago

Commit: 159ec4b

fchapoton commented 3 years ago
comment:4

first tentative

New commits:

159ec4bfirst step towards using argparse for doc command line arguments
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

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

56ce7b6more work on argparse for doc command line
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 159ec4b to 56ce7b6

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 56ce7b6 to 98824cd

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

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

98824cdMerge branch 'public/ticket/31366' in 9.5.b1
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

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

d9cd177Merge branch 'public/ticket/31366' in 9.5.b5
ec995b7some details
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 98824cd to ec995b7

fchapoton commented 3 years ago

Author: Frédéric Chapoton

fchapoton commented 3 years ago
comment:8

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

fchapoton commented 3 years ago
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'
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from ec995b7 to 3a4c572

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

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

3a4c572more work on argparse for docbuild
fchapoton commented 3 years ago

Description changed:

--- 
+++ 
@@ -1 +1,3 @@
 Sage's docbuilding uses [optparse](https://docs.python.org/3/library/optparse.html) for argument parsing, but this library has been deprecated for a while. We should switch to [argparse](https://docs.python.org/3/library/argparse.html).
+
+Some instructions for conversions can be found here: https://docs.python.org/3/library/argparse.html#help
jhpalmieri commented 3 years ago
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?

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

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

70b6aa6another fix for argparse in docbuild
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 3a4c572 to 70b6aa6

fchapoton commented 3 years ago
comment:14

here is a tentative. I had no time to check

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

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

914ce7dmore work on argparse for docbuild
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 70b6aa6 to 914ce7d

fchapoton commented 3 years ago
comment:16

more fixes, but apparently not ok yet

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 914ce7d to 199f8a0

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

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

199f8a0trac 31366: more fixes
jhpalmieri commented 3 years ago
comment:18

I tried implementing this a while ago (before your branch), so here are some contributions to your branch from my attempts. They help, but not sure if they are good enough yet.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

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

1745efbpolishing argparse for docbuild
84bde61Merge branch 'public/ticket/31366' in jhpalmieri branch
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 199f8a0 to 84bde61

fchapoton commented 3 years ago
comment:20

I was working on that right now. I had to merge my branch with yours, hopefully in a correct way. Starts to look better.

jhpalmieri commented 3 years ago
comment:21

Yes, much better. We have to figure how to allow commands like ./sage --docbuild -h, which currently says

usage: sage --docbuild [OPTIONS] DOCUMENT (FORMAT | COMMAND)
__main__.py: error: the following arguments are required: document, format
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 84bde61 to 8c8fc39

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

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

8c8fc39still working on argparse for docbuilding
fchapoton commented 3 years ago
comment:23

more progress done.

fchapoton commented 3 years ago
comment:24

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

jhpalmieri commented 3 years ago
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)
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

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

00d94fffix for empty input
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 8c8fc39 to 00d94ff

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

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

b02c912fine tuning details in argparse for docbuild
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 00d94ff to b02c912

fchapoton commented 3 years ago
comment:28

here is a new tentative

jhpalmieri commented 3 years ago
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.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

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

c6e7bdatrac 31366: minor fixes to help_examples
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from b02c912 to c6e7bda

jhpalmieri commented 3 years ago
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 commented 2 years ago
comment:32

Ready for review? I am happy with your changes.

jhpalmieri commented 2 years ago

Reviewer: John Palmieri

fchapoton commented 2 years ago
comment:34

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

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from c6e7bda to 0b62c16