sifive / wit

Workspace Integration Tool
Apache License 2.0
23 stars 13 forks source link

Refactor parser #193

Closed jackkoenig closed 4 years ago

jackkoenig commented 4 years ago

Including @mwachs5 as a reviewer because the main purpose of this PR is improving error messages that hopefully will help her and other users.

Each commit is meaningful and I have questions on all of them (except the first)

  1. Split command-line parser into different file

This one is simple, but it just splits out the exact code from main.py to parser.py. The one thing I don't like is that everything is top-level and mutable, I'd rather have some immutable representation of the parser and each of the subparsers, but that seems pretty difficult to do, so this will do for now I guess.

  1. Improve parser help text

This improves how subcommands are specified. This changes it from:

$ wit -h
usage: __main__.py [-h] [-v] [--version] [-C path] [--repo-path REPO_PATH]
                   [--prepend-repo-path PREPEND_REPO_PATH]
                   {init,add-pkg,update-pkg,add-dep,update-dep,status,update,inspect,fetch-scala}
                   ...

positional arguments:
  {init,add-pkg,update-pkg,add-dep,update-dep,status,update,inspect,fetch-scala}
                        sub-command help
    init                create workspace
    add-pkg             add a package to the workspace
    update-pkg          update the revision of a previously added package
    add-dep             add a dependency to a package
    update-dep          update revision of a dependency in a package
    status              show status of workspace
    update              update git repos
    inspect             inspect lockfile
    fetch-scala         Fetch dependencies for Scala projects

optional arguments:
  -h, --help            show this help message and exit
  -v, --verbose         Specify level of verbosity
                        -v:    verbose
                        -vv:   debug
                        -vvv:  trace
                        -vvvv: spam
  --version             Print wit version
  -C path               Run in given path
  --repo-path REPO_PATH
                        Specify alternative paths to look for packages
  --prepend-repo-path PREPEND_REPO_PATH
                        Prepend paths to the default repo search path.

To:

$ wit -h
usage: wit [-h] [-v] [--version] [-C path] [--repo-path REPO_PATH]
           [--prepend-repo-path PREPEND_REPO_PATH]
           <command> ...

optional arguments:
  -h, --help            show this help message and exit
  -v, --verbose         Specify level of verbosity
                        -v:    verbose
                        -vv:   debug
                        -vvv:  trace
                        -vvvv: spam
  --version             Print wit version
  -C path               Run in given path
  --repo-path REPO_PATH
                        Specify alternative paths to look for packages
  --prepend-repo-path PREPEND_REPO_PATH
                        Prepend paths to the default repo search path.

subcommands:
  <command>             <description>
    init                create workspace
    add-pkg             add a package to the workspace
    update-pkg          update the revision of a previously added package
    add-dep             add a dependency to a package
    update-dep          update revision of a dependency in a package
    status              show status of workspace
    update              update git repos
    inspect             inspect lockfile
    fetch-scala         Fetch dependencies for Scala projects
  1. Improve help text for add_dep_parser

Changes it from:

$ wit add-dep -h
usage: __main__.py add-dep [-h] pkg[::revision]

positional arguments:
  pkg[::revision]

optional arguments:
  -h, --help       show this help message and exit

To:

$ wit add-dep -h
usage: wit add-dep [-h] pkg[::revision]

Adds <pkg> as a dependency to a target package determined by the current
working directory (which can be set by -C)

positional arguments:
  pkg[::revision]  revision can be any git commit object, default is currently
                   checked out commit

optional arguments:
  -h, --help       show this help message and exit
  1. Use add_dep_parser for help instead of custom error

Related to https://github.com/sifive/wit/pull/187, this uses the help text for the add_dep_parser instead of the text we added in #187, it's a bit DRY-er which is nice and could set a pattern for future similar error messages.

Before:

$ wit add-dep foo
[ERROR] add-dep must be run inside of a package, not the workspace root.
  A dependency is added to the package determined by the current working directory,
  which can also be set by -C.

After:

$ wit add-dep foo
[ERROR] add-dep must be run inside of a package, not the workspace root.

usage: wit add-dep [-h] pkg[::revision]

Adds <pkg> as a dependency to a target package determined by the current
working directory (which can be set by -C)

positional arguments:
  pkg[::revision]  revision can be any git commit object, default is currently
                   checked out commit

optional arguments:
  -h, --help       show this help message and exit

Perhaps this is Much Ado About Nothing but I'm looking for patterns to apply going forward, and I think there are some good ideas here. I would just like to check before applying these ideas any further.

jackkoenig commented 4 years ago

I'll add some delimiter comments, thanks!