anntzer / defopt

Effortless argument parser
https://pypi.org/project/defopt/
MIT License
214 stars 11 forks source link

Add recursive subcommands #90

Closed Spectre5 closed 3 years ago

Spectre5 commented 3 years ago

Here is an initial take on using recursion for nested subcommands. I've added tests but I have not well tested it in a "real" application yet.

The docs (help message) as briefly discussed in #89 is not implemented at this point but I do like the idea of adding a __doc__ or similar method to add help at the "higher level" subcommands.

This implementation also requires the subcommands to be mutable mappings so that the key can be used as the subcommand name. I'm not sure where the subcommand name would come from if nested lists were used. Perhaps a "magic" function with __doc__ in the name could be used for that too, if it is desired to support it.

Closes #89

anntzer commented 3 years ago

This implementation also requires the subcommands to be mutable mappings so that the key can be used as the subcommand name.

Can you make sure a meaningful error is raised otherwise?


I haven't tried to debug things, but

import defopt

def a(): pass
def b(): pass
def c(): pass
defopt.run({"foo": [a], "bar": [b, c]}, argv=["--help"])

gives

usage: test.py [-h] {foo} ...

positional arguments:
  {foo}

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

which seems wrong? ("bar" is missing)

Spectre5 commented 3 years ago

I've added tests for the issue you found and fixed it. It was due to a naming conflict. I'll address changing the test in test_nested_subcommands_mixed to make sure it fails instead a little later.

Spectre5 commented 3 years ago

The latest commit disallows mappings within a list. See the "test_nested_subcommands1", which now must fail, along with "test_nested_subcommands_mixed".

anntzer commented 3 years ago

Sorry I'm changing things under the rug at the same time. Can you rebase your PR on top of master instead? (I can force push to your branch if you prefer.)

Spectre5 commented 3 years ago

I did merge in master and I don't see any newer commits. What else should be added? I did add a dead-simple example that could potentially be expanded and I added an entry to the changelog. I guess a new section to the features section of the docs could be added too?

anntzer commented 3 years ago

Don't worry about the commit history, I'll clean it up when merging the PR.

anntzer commented 3 years ago

A blurb in features.rst (e.g. under Subcommands) would be nice, yes.

anntzer commented 3 years ago

Looks good to me except for one nit. Also, do you want to squash your commits yourself or should I do it?

Spectre5 commented 3 years ago

I added the link to the example, added a nested lists example (that should fail), and updated the value error message that is raised when converting funcs to a list to read "Use dictionaries (mappings) for nesting; other iterables may only contain functions (callables)." I think this is slightly more clear to explicit say to use dictionaries for nesting. The previous message raised here indicated that a dictionary cannot be in a list --- but as the newly added test shows, a list obviously cannot be in a list either!

anntzer commented 3 years ago

Thanks, merged :)