Opened 17 months ago

Closed 7 months ago

#31366 closed enhancement (fixed)

docbuild: switch from optparse to argparse

Reported by: jhpalmieri Owned by:
Priority: major Milestone: sage-9.5
Component: documentation Keywords:
Cc: Merged in:
Authors: Frédéric Chapoton Reviewers: John Palmieri
Report Upstream: N/A Work issues:
Branch: 0b62c16 (Commits, GitHub, GitLab) Commit: 0b62c166044271f0b7db4f42bb174e560562a364
Dependencies: #32946 Stopgaps:

Status badges

Description (last modified by chapoton)

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

Change History (37)

comment:1 Changed 17 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

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

comment:2 Changed 12 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

comment:3 Changed 11 months ago by chapoton

a little bit in #32331

comment:4 Changed 11 months ago by chapoton

  • Branch set to public/ticket/31366
  • Commit set to 159ec4b5b77cc7b3b5a008a08d01a691ed506b3f

first tentative


New commits:

159ec4bfirst step towards using argparse for doc command line arguments

comment:5 Changed 11 months ago by git

  • Commit changed from 159ec4b5b77cc7b3b5a008a08d01a691ed506b3f to 56ce7b68910f0f6a8e00c58239fa81a2bd0c1fd2

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

56ce7b6more work on argparse for doc command line

comment:6 Changed 10 months ago by git

  • Commit changed from 56ce7b68910f0f6a8e00c58239fa81a2bd0c1fd2 to 98824cd9e4d5719358630e25305aee6dffd9c30f

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

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

comment:7 Changed 8 months ago by git

  • Commit changed from 98824cd9e4d5719358630e25305aee6dffd9c30f to ec995b76d9096379648f1a78decc0569819e980a

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

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

comment:8 Changed 8 months ago by chapoton

  • Authors set to Frédéric Chapoton
  • Status changed from new to needs_review

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

comment:9 Changed 8 months ago by chapoton

  • Status changed from needs_review to needs_work

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'

comment:10 Changed 8 months ago by git

  • Commit changed from ec995b76d9096379648f1a78decc0569819e980a to 3a4c572d5c7be610c632cf2ae66ba436636412c1

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

3a4c572more work on argparse for docbuild

comment:11 Changed 8 months ago by chapoton

  • Description modified (diff)

comment:12 Changed 8 months ago by jhpalmieri

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?

Last edited 8 months ago by jhpalmieri (previous) (diff)

comment:13 Changed 8 months ago by git

  • Commit changed from 3a4c572d5c7be610c632cf2ae66ba436636412c1 to 70b6aa64b80ad7ff159fc4921a8dedbfb7096e29

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

70b6aa6another fix for argparse in docbuild

comment:14 Changed 8 months ago by chapoton

here is a tentative. I had no time to check

comment:15 Changed 8 months ago by git

  • Commit changed from 70b6aa64b80ad7ff159fc4921a8dedbfb7096e29 to 914ce7d900553b87d59fc27e3f98ca16adfa93d2

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

914ce7dmore work on argparse for docbuild

comment:16 Changed 8 months ago by chapoton

more fixes, but apparently not ok yet

comment:17 Changed 8 months ago by git

  • Commit changed from 914ce7d900553b87d59fc27e3f98ca16adfa93d2 to 199f8a03e9907b71453730e540933e6b5997d5e6

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

199f8a0trac 31366: more fixes

comment:18 Changed 8 months ago by jhpalmieri

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.

comment:19 Changed 8 months ago by git

  • Commit changed from 199f8a03e9907b71453730e540933e6b5997d5e6 to 84bde616d574537bf92eb77e1b7512942fe042e5

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

1745efbpolishing argparse for docbuild
84bde61Merge branch 'public/ticket/31366' in jhpalmieri branch

comment:20 Changed 8 months ago by chapoton

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

comment:21 Changed 8 months ago by jhpalmieri

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

comment:22 Changed 8 months ago by git

  • Commit changed from 84bde616d574537bf92eb77e1b7512942fe042e5 to 8c8fc39b7cdc72658c07425991d5c5738f561899

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

8c8fc39still working on argparse for docbuilding

comment:23 Changed 8 months ago by chapoton

more progress done.

comment:24 Changed 8 months ago by chapoton

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

comment:25 Changed 8 months ago by jhpalmieri

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.

  • src/sage_docbuild/__init__.py

    diff --git a/src/sage_docbuild/__init__.py b/src/sage_docbuild/__init__.py
    index 1f692f1abe..84a6444ec1 100644
    a b def help_description(s="", compact=False): 
    13851385    character.
    13861386    """
    13871387    s += "Build or return information about Sage documentation.\n\n"
    1388     s += "    DOCUMENT    name of the document to build\n"
    1389     s += "    FORMAT      document output format\n"
    1390     s += "    COMMAND     document-specific command\n\n"
    13911388    s += "Note that DOCUMENT may have the form 'file=/path/to/FILE',\n"
    13921389    s += "which builds the documentation for the specified file.\n\n"
    13931390    s += "A DOCUMENT and either a FORMAT or a COMMAND are required,\n"
    def setup_parser(): 
    16251622                          help="if ARG is 'reference', list all subdocuments"
    16261623                          " of en/reference. If ARG is 'all', list all main"
    16271624                          " documents")
    1628     parser.add_argument("document", nargs='?', type=str)
    1629     parser.add_argument("format", nargs='?', type=str, choices=get_formats())
     1625    parser.add_argument("document", nargs='?', type=str,
     1626                        help='name of the document to build')
     1627    parser.add_argument("format", nargs='?', type=str, choices=get_formats(),
     1628                        help='document output format')
    16301629    return parser
    16311630
    16321631
    def main(): 
    17221721    # trying to build.
    17231722    name, typ = args.document, args.format
    17241723    if not name or not type:
    1725         raise ValueError('both document and format should be given')
     1724        parser.print_help()
     1725        sys.exit(1)
    17261726
    17271727    # Set up module-wide logging.
    17281728    setup_logger(args.verbose, args.color)

comment:26 Changed 8 months ago by git

  • Commit changed from 8c8fc39b7cdc72658c07425991d5c5738f561899 to 00d94ffb760d49b6e926be00d27a0d0da0d84cff

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

00d94fffix for empty input

comment:27 Changed 8 months ago by git

  • Commit changed from 00d94ffb760d49b6e926be00d27a0d0da0d84cff to b02c912a2181b385f4bf0413599da9eb659a48b3

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

b02c912fine tuning details in argparse for docbuild

comment:28 Changed 8 months ago by chapoton

here is a new tentative

comment:29 Changed 8 months ago by jhpalmieri

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.

comment:30 Changed 8 months ago by git

  • Commit changed from b02c912a2181b385f4bf0413599da9eb659a48b3 to c6e7bdaee8f214410e321fae87a8a0279ae2c7b8

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

c6e7bdatrac 31366: minor fixes to help_examples

comment:31 Changed 8 months ago by jhpalmieri

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).

comment:32 Changed 7 months ago by jhpalmieri

Ready for review? I am happy with your changes.

comment:33 Changed 7 months ago by jhpalmieri

  • Reviewers set to John Palmieri

comment:34 Changed 7 months ago by chapoton

  • Status changed from needs_work to needs_review

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

comment:35 Changed 7 months ago by git

  • Commit changed from c6e7bdaee8f214410e321fae87a8a0279ae2c7b8 to 0b62c166044271f0b7db4f42bb174e560562a364

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

comment:36 Changed 7 months ago by jhpalmieri

  • Dependencies set to #32946
  • Status changed from needs_review to positive_review

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

comment:37 Changed 7 months ago by vbraun

  • Branch changed from public/ticket/31366 to 0b62c166044271f0b7db4f42bb174e560562a364
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.