Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding toggling decorators and testing #73

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nrhinehart
Copy link

Adds decorators that enable the creation of pairs of mutually exclusive boolean arguments.

@neithere
Copy link
Owner

(See #72 for details.)

At the moment I cannot accept this PR as it doesn't pass tests:

  1. Python 2.6 doesn't understand '{}' ('{0}' is required).
  2. The tests for Python 2.7 had failed because of a connection problem so I've restarted the build. Hopefully it will pass. Travis is soooooo slow these days.

@nrhinehart
Copy link
Author

The Python 2.6 formatting issue has been fixed.

@neithere
Copy link
Owner

Cool, thanks. I'm now reading your pull request thoroughly which also results in light refactoring and adding more detailed tests. And it seems that we've dug up a small problem which was given not enough attention before.

The question is: what should we do if the default value of the toggleable argument is True? Let's take this example:

def cmd(a=True, b=False):
    print('a', a, 'b', b)

How does it behave? The store_x actions are assigned by negating the default values, so the defaults are kept intact when the related argument is missing but inverted when the argument is supplied:

$ cmd
a True b False
$ cmd -a -b
a False b True

Note that this is purely on the CLI level; Python does not support the notion of switches in function calls and requires a value along with the argument name, so consistency with Python notation is irrelevant here.

Alright, now if we mark these arguments as toggleables, how should they behave? I would expect them to behave exactly the same way with the only difference being that --no-foo arguments are added (mutually exclusive with their positive counterparts). That is, the overall behaviour should be consistent regardless of whether the argument was added to the list of toggleables or not.

But then two of your tests (multiarg and multitoggle) would fail because they suppose a different behaviour: --foo implies store_true and --no-foo implies store_false regardless of the argument's default value.

And the more I think about which is right and which is wrong, the more I lean towards your version.

Without toggleables it was pretty evident that cmd(x=False) should be 'store_true' and cmd(x=True) should be 'store_false' as there was no other way to invert the value from CLI. But now there is one, and the benefit of having a cmd(foo=True) with a --foo that yields False becomes highly questionable.

It is indeed very strange for a --not-foo to yield a True value for foo argument. Maybe it's up to the developer to decide if the argument should deviate from the common idiom foo=False for a switch; however, I cannot come up with a sensible use case.

Another example:

def cmd(table=True):
    if table:
        print('table rows')
    else:
        print('lines')

The corresponding CLI:

$ cmd
table rows
$ cmd --table
lines

WTF?! Of course we'd have to rename the argument to no_table/--no-table, but then the code becomes overcomplicated with these double negations. The only sensible version is this one:

$ cmd
table rows
$ cmd --table
table rows
$ cmd --no-table
lines

...And it should correspond to a function signature cmd(table=True).

So, it seems to me that when argument metadata is inferred from the function and the default value is a bool, the action should always become 'store_true'; but if that bool value is also True, we should also add a "counterargument" --no-x. This would move your feature a bit closer to the core of the library. This requires a bit more thinking and refactoring but it seems that writing this comment helped me understand a lot about the issue :)

Anyway, what I mentioned as preferable implies backward incompatibility; we may simply add the decorator and then deprecate it for certain cases when the desired behaviour could be triggered automatically.

@nrhinehart
Copy link
Author

Thanks for the response. I created these decorators because of the problems you mentioned -- nonintuitive behavior (also with using argparse) that can result when there is a disconnect between the names of boolean parameters and their default behavior, and the increased mental load that places on the developer when invoking a program. After using the feature for a few months myself, I've found that I'm always using @argh.set_all_toggleable()

Basically, there is never a case when I don't want a mutually exclusive pair for boolean arguments, because then I can always be guaranteed to get what I want from my programs by specifying one of --foo or --no-foo

The behavior you mentioned for "counterargument" creation is interesting, but it might be confusing if both the decorators and that exist.

Another option could be to make adding the toggling pairs the default behavior, and instead make a separate decorator to turn it off, in which case, all inference of parameter meaning should be left to the developer who turned off the toggling for a(ny) parameter(s). However, that may or may not be too strict and biased toward my own use preferences >:)

@neithere
Copy link
Owner

make adding the toggling pairs the default behavior, and instead make a separate decorator to turn it off, in which case, all inference of parameter meaning should be left to the developer who turned off the toggling for a(ny) parameter(s)

This is exactly what I meant. I think the treatment of booleans should be smarter than it is (and it already is smarter than it was with barebones argparse, so we shouldn't be afraid of it in general).

I would even think of a simplified default behaviour:

  • cmd(foo=False)cmd [--foo]
  • cmd(foo=True)cmd [--no-foo]

...and a method (dedicated decorator or a keyword in a decorator) to enable redundant arguments (that don't have effect) to make these mutually exclusive pairs of --foo/--no-foo.

It would look like this:

# negative defaults, action 'store_true'

# --foo is autotoggled
@toggle('--bar')    # use redundant negative counterargument
@toggle('--quux', 'not')    # same, also specify custom prefix
def cmd_a(foo=False, bar=False, quux=False):
    """
    usage: cmd-a [--foo] [--bar | --no-bar] [--quux | --not-quux]
    """
    pass

# positive defaults, action 'store_false'

# --foo is autotoggled
@toggle('--bar')    # use redundant positive counterargument
@toggle('--quux', 'not')    # same, also specify custom prefix
@toggle('--whiz', 'never', redundant=False)    # custom prefix but no redundancy
def cmd_b(foo=True, bar=True, quux=True, whiz=True):
    """
    usage: cmd-b [--no-foo] [--bar | --no-bar] [--quux | --not-quux] [--never-whiz]
    """
    pass

I think cmd(foo=True) mapped to cmd --foo is a marginal (and probably even harmful) case which is safe to omit.

Of course a @toggle_all decorator would also make sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants