larryhastings / appeal

Command-line parsing library for Python 3.
Other
122 stars 7 forks source link

Improve the error message generated when a required parameter appears after a converter with a VAR_POSITIONAL parameter #6

Closed asfaltboy closed 1 year ago

asfaltboy commented 1 year ago

Thank's for writing appeal it's a cool package, and the README is a pleasure to read 🥇

I've come across an issue with using the pathlib.Path type, since I wanted to specify path arguments with the rich pathlib API, and I encountered this awkward issue.

Example:

import pathlib

import appeal

app = appeal.Appeal()

@app.command()
def paths_exist(path1: pathlib.Path, path2: pathlib.Path):
    exists_result1 = "exists" if path1.exists() else "does not exist"
    print(f"path1 {exists_result1}")
    exists_result2 = "exists" if path2.exists() else "does not exist"
    print(f"path2 {exists_result2}")

if __name__ == "__main__":
    app.main()

Adding an annotation in the first argument raises an error:

$ python cli.py

Traceback (most recent call last):
  File "./appeal_test/cli.py", line 17, in <module>
    app.main()
  File "./venv/lib/python3.10/site-packages/appeal/__init__.py", line 4203, in main
    sys.exit(self.process(args))
  File "./venv/lib/python3.10/site-packages/appeal/__init__.py", line 4179, in process
    return self.help()
  File "./venv/lib/python3.10/site-packages/appeal/__init__.py", line 4045, in help
    appeal.usage(usage=True, summary=True, doc=True)
  File "./venv/lib/python3.10/site-packages/appeal/__init__.py", line 4001, in usage
    usage_str, summary_str, doc_str = self.render_docstring(commands=self.commands, override_doc=docstring)
  File "./venv/lib/python3.10/site-packages/appeal/__init__.py", line 3897, in render_docstring
    usage_str, split_summary, doc_sections = self.compute_usage(commands=commands, override_doc=override_doc)
  File "./venv/lib/python3.10/site-packages/appeal/__init__.py", line 3310, in compute_usage
    child.analyze()
  File "./venv/lib/python3.10/site-packages/appeal/__init__.py", line 4084, in analyze
    self._analyze_attribute("_global")
  File "./venv/lib/python3.10/site-packages/appeal/__init__.py", line 4076, in _analyze_attribute
    program = charm_compile(self, callable)
  File "./venv/lib/python3.10/site-packages/appeal/__init__.py", line 1398, in charm_compile
    program = cc.compile(callable, default, is_option=is_option)
  File "./venv/lib/python3.10/site-packages/appeal/__init__.py", line 1255, in compile
    pg = argument_grouping.ParameterGrouper(callable, default, signature=signature)
  File "./venv/lib/python3.10/site-packages/appeal/argument_grouping.py", line 451, in __init__
    self.required, self.optional = pgf.analyze()
  File "./venv/lib/python3.10/site-packages/appeal/argument_grouping.py", line 263, in analyze
    return self.third_pass()
  File "./venv/lib/python3.10/site-packages/appeal/argument_grouping.py", line 232, in third_pass
    raise ValueError(f'Required parameter "{breadcrumb}.{p}" found after VAR_POSITIONAL parameter "{found_var_positional}"')
ValueError: Required parameter "paths_exist.path2" found after VAR_POSITIONAL parameter "paths_exist.path1.args."

Removing the first annotation, and only keeping the annotation in the 2nd argument (and possibly any next params) doesn't raise an error but changes the argument to appear as [args]... in the usage line (even though it's a single argument, and not a *arg.

Using appearl 0.5 from pypi on python 3.10

larryhastings commented 1 year ago

I admit the error is inscrutable. But it's legitimate and Appeal is working correctly.

The problem is the signature of pathlib.Path:

def __new__(cls, *args, **kwargs):
    ...

Since the constructor for a pathlib.Path object will take infinitely many positional arguments, there's no way for any subsequent argument to your command function to accept a positional argument. So Appeal throws an exception.

You could work around it by writing your own function, something like this:

def mypath(path):  return pathlib.Path(path)
@appeal.command()
def paths_exist(path1: mypath, path2: mypath):
    ...

I'll leave the issue open for now, so that I can reflect on how to improve the error message.

larryhastings commented 1 year ago

What do you think of

This command-line can never be satisfied.  Required parameter "paths_exist.path2" can never get an argument, because it comes after "paths_exist.path1", which is type "Path", and that has a "*args" parameter, which means it consumes all remaining command-line arguments

It's a little long, but it sure explains everything.

larryhastings commented 1 year ago

Fixing this in 0.5.9. The error message I went with is (manually word-wrapped):

ValueError: This command-line can never be satisfied.  "fgrep(), argument dst" is a required parameter,
but it can never get an argument, because it comes after VAR_POSITIONAL parameter
"fgrep(), argument src, converter Path(), argument *args" which already consumed
all remaining command-line arguments.