0.30.X is breaking console scripts by moving options to positional arguments

[valentin-feron]

My CI pipelines broke after 0.30.X release. The options of my python scripts are now positional arguments.

output of the helper of one of my tool with argh 0.29.3:

positional arguments:
  commit_message        -

options:
  -h, --help            show this help message and exit
  -w WORK_TREE, --work-tree WORK_TREE
                        '.'
  -g GIT_DIR, --git-dir GIT_DIR
                        -
  -a, --auto-choose-default-action
                        False
  --bypass-docstring-validation
                        False
  --bypass-tests        False
  --bypass-code-formatting
                        False
  -v, --verbose         False

Output of the same helper with argh 0.30.1:

positional arguments:
  commit-message        -
  work-tree             '.'
  git-dir               -
  auto-choose-default-action
                        False
  bypass-docstring-validation
                        False
  bypass-tests          False
  bypass-code-formatting
                        False
  verbose               False

options:
  -h, --help            show this help message and exit

Here is the signature of the python function:

def check_in(
    commit_message: str,
    work_tree='.',
    git_dir=None,
    auto_choose_default_action=False,
    bypass_docstring_validation=False,
    bypass_tests=False,
    bypass_code_formatting=False,
    verbose=False,
):

After doing the following, I get the options back:

def check_in(
    commit_message: str,
    *,
    work_tree='.',
    git_dir=None,
    auto_choose_default_action=False,
    bypass_docstring_validation=False,
    bypass_tests=False,
    bypass_code_formatting=False,
    verbose=False,
):

It is not a bug, since a choice has been made to change how "positional or keyword" args are interpreted, switching from options (except for the first arg) to positional arguments. But it will break a lot of user's commands, as it did for me.

Maybe adding a warning when wrapping a function that contains "positional or keyword" args would be enough (it would have made me save some precious time actually), encouraging the user to use "positional only" and "keyword only" args.

[valentin-feron]

With another function, I got this error

argh.exceptions.AssemblingError: make_autodocs: argument "ignore" declared as positional (in function signature) and optional (via decorator). If you've just migrated from Argh v.0.29, please check the new default NameMappingPolicy. Perhaps you need to replace `func(x=1)` with `func(*, x=1)`?

The signature is:

@argh.arg('-i', '--ignore', nargs='*')
def make_autodocs(
    project_dir: Union[str, Path],
    output_dirname='module_docs',
    skip_existing=True,
    docsrc_dir=None,
    ignore: List[str] = None,
):

Here, raising an exception makes sense, because of the inconsistency between the type of arg and the decorator. But I wanted to point it out because the error message is very helpful here. Maybe we want something similar (in a warning though) when using "positional or keyword" args.

[neithere]

Hi @valentin-feron, thanks for the report!

What you are saying makes total sense.

The problem is that func(foo=1, *, bar=2): ... is still valid in terms of the new name mapping policy: it means func [foo] [--bar BAR]. So if we issue a warning for func(foo=1), we potentially pollute the output for a totally valid use case. On the other hand, that use case is marginal.

Perhaps issuing the warning on default value in non-kwonly args and introducing something like set_default_command(..., suppress_name_mapping_policy_warnings=True) would be a decent solution during a certain transitional period.

What do you think?

[valentin-feron]

I understand you don't want to pollute the output. And I don't think this a marginal use case. Actually I think this is a very common use case. I personally tend to do func(foo=1, *, bar=2, ...) when I got a "main" arg plus 4 or 5 "extra" parameters instead of doing func(foo=1, /, *, bar=2), so I give the user the choice of defining foo the way he wants, with kw or not. But anyway, I am not a python master jedi, so take this assumption with a grain of salt.

The solution you propose gives the user control on displaying the warning message or not, but once the transitional period is over, you got an extra parameter that you cannot delete (because people will have it set to True) or reuse (because you'll want other future warnings to be displayed by default).

Now, if we think about it, the choice of mapping non-kwonly args to positional cmd arguments is arbitrary. So, what about making the user decide? We could have something like that:

def set_default_command(
    ...
    pos_or_kw_args_policy=None
)
...

By default, and during transitional period, default value for pos_or_kw_args_policy is None, meaning that if the user tries to wrap a function with pos_or_kw args (or non-kwonly if you want), it raises an exception with a useful explanation, inviting the user to define pos_or_kw_args_policy. Then, once the transitional period is over, change the default value to POSITIONAL_ARG so it is mapped to positional cmd args by default. This way, you give the user control and don't end up with a useless parameter in set_default_command. One big con though, is that it will break the code of any user using pos_or_kw args (like me), obliging them to make a choice to fix the error.

What do you think?

[neithere]

I think this is a very common use case

By marginal case I meant the situation when the user specifically wants a positional CLI argument with nargs="?". This is when the warning would appear.

... instead of doing func(foo=1, /, *, bar=2)...

Good point. The new name mapping policy essentially interprets func(foo, bar=1, *, baz, quux=2) as func(foo, bar=1, /, *, baz, quux=2) to produce func foo [bar] --baz BAZ [--quux QUUX].

It would be interesting to map two CLI arguments to each positional-or-keyword func arg — one positional and one option — and let the user choose how they want to call it, i.e. func [--foo FOO] [foo]. However, I don't think argparse is that flexible, especially when it comes to required arguments. Using mutually exclusive groups for this seems like overkill. So I don't think we can fully preserve the flexibility of Python arguments in the mapping. Perhaps this should be better documented.

an extra parameter that you cannot delete (because people will have it set to True)

I suppose it's not a big issue: people should set it to true when they know what they are doing — not just snoozing the alarm but saying "I know about the issue and it's not applicable in my case, I'm using it correctly" (in that nargs="?" scenario).

By default, and during transitional period, default value for pos_or_kw_args_policy is None, meaning that if the user tries to wrap a function with pos_or_kw args (or non-kwonly if you want), it raises an exception with a useful explanation

That's close to my original intent (#191, see Transition): start showing a warning if the policy is not chosen explicitly, then at some point set the new policy as the default one. In this case this would throw an exception instead of the warning. On one hand, this would completely break everything for everyone. On the other hand, I see that it does so indeed, unfortunately, so it would be better to fail more explicitly and show instructions.

inviting the user to define pos_or_kw_args_policy

As I understand, that would mean requesting every user to set set_default_command(..., name_mapping_policy=NameMappingPolicy.BY_NAME_IF_KWONLY) or something like that. And then at some point this change would become unnecessary, and the users would get a deprecation warning and would have to edit the code again. I'd rather avoid this.

I'd probably try the following:

• If the policy was chosen explicitly
  • we assume that the developer knows what they are doing and don't issue any warnings.
• else
  • If we see a positional-or-keyword argument with a default value,
  • then that's probably an earlier attempt to declare an option (--foo)
  • Throw an exception suggesting do one of the following: a) add the *, or b) set the legacy policy explicitly.

[neithere]

The exception with detailed instructions will be raised starting with v0.30.2 (just released).

Thanks again for your input, @valentin-feron! I'm sure it has saved time for other developers.