Make Optional[bool] still act as a flag

[Spectre5]

This is an initial implementation of the feature requested in #99.

I may not have thought through all of the potential cases here, so likely this testing would need to be expanded and the implementation thought through more closely. But first I'd like to see if there is interest in it.

Using the code from the linked issue/feature request, it would now function as below (specifically note the line skip = X):

$ python dummy.py
loops = 5
skip  = None
long loop iteration: 1
long loop iteration: 2
long loop iteration: 3
long loop iteration: 4
long loop iteration: 5
$ python dummy.py --skip
loops = 5
skip  = True
skipping loop printing
$ python dummy.py --no-skip
loops = 5
skip  = False
long loop iteration: 1
long loop iteration: 2
long loop iteration: 3
long loop iteration: 4
long loop iteration: 5

Closes #99

[anntzer]

Good catch, this seems like a complete regression introduced by https://github.com/anntzer/defopt/commit/ac198e8 (which was implemented in response to #87, which seems like a reasonable request in general).

I think this just needs a changelog entry, and a bit of text expanding on the block in features.rst by that commit to explain the special-casing of bools (no need for a long rationale, just say that bools behave differently wrt Optional).

Extra brownie points if you additionally check what happens if a parser is provided for bool (e.g. one could decide instead to parse 1/t/true as True and 0/f/false as False), and, if that's indeed supported, have Optional[bool] behave as for normal types.

[Spectre5]

One more thing to consider as well is what to do if an Optional[bool] is used with no negated flags. Then you effectively cannot get the False value, only True or None. I suppose that could still be desired if the CLI function is also used outside of defopt, but it seems unlikely that it would generally be the intent. Nevertheless, that is how it currently works and it seems reasonable. Otherwise, if desired, we can disable the no negated flag in this case, similar to how it gets disable for a normal boolean if the default is True.

[Spectre5]

Extra brownie points if you additionally check what happens if a parser is provided for bool (e.g. one could decide instead to parse 1/t/true as True and 0/f/false as False), and, if that's indeed supported, have Optional[bool] behave as for normal types.

Can you explain this part more? What exactly do you mean here, to remove them as flags and instead use --skip t or --skip f, for example? I'd personally rather have --skip and --no-skip for an optional bool, but I'm probably misunderstanding your intent here.

Edit: Ah, nevermind, I see what you mean. I'll check into it!

[Spectre5]

I think that just works:

from typing import Optional

import defopt

def _parse_none(string):
    if string.lower() == 'none':
        return None
    else:
        raise ValueError('{} is not a valid None string'.format(string))

def test(loops: int = 5, skip: Optional[bool] = None):
    """This is a dummy function.

    Parameters
    ----------
    skip
        Skip printing something.
    """
    print(f'loops = {loops}')
    print(f'skip  = {skip}')
    _skip = skip if skip is not None else loops > 10
    if _skip:
        print(f'skipping loop printing')
    for i in range(loops):
        if not _skip:
            print(f'long loop iteration: {i+1}')

if __name__ == '__main__':
    defopt.run(test, parsers={type(None): _parse_none})

$ python dummy.py 5 t
loops = 5
skip  = True
$ python dummy.py 5 None
loops = 5
skip  = None
long loop iteration: 1
long loop iteration: 2
long loop iteration: 3
long loop iteration: 4
long loop iteration: 5

[Spectre5]

And when used as a keyword only argument, I don't think you even can specify a bool parser for either a bool or Optional[bool] type as it would get consumed before the parser was accessed here:

https://github.com/anntzer/defopt/blob/0e9032c5cdfc8a9ca9cfcd6a64317ad8d84e21bf/lib/defopt.py#L448

[anntzer]

OK, I think this looks reasonable (just needs changelog entries and updating features.rst). Let's treat this as a bugfix (as noted above) and think about interactions with no_negated_flags and custom bool parsers another time.

[Spectre5]

Ok, I will make those updates soon.

As for the interactions with the parsers, can you describe more about what your idea or ideal scenario is there? I won't incorporate it in this PR but will keep it in mind if I have some time in the future to work it. To keep that discussion separate, maybe open it as a new issue?

[anntzer]

(I'm off until new year, will continue this discussion after that. Happy holidays :))

[Spectre5]

Sounds good. Check out the doc updates (features and changelog) and let me know how you'd like it edited. ~For the changelog, I moved all previous entries to 6.2.0 and this is the only new one under the next section now. I assume this is correct, but please double check it~ Scratch that, I see it was updated separately now.

[Spectre5]

Done. Just curious, why don't you use the "squash and merge" option in GitHub when merging it?

[Spectre5]

Oops, restored this branch

[anntzer]

Mostly out of habit, but also to allow the contributor to still split the work in a couple of logical commits if the patch is really large and separate commits make sense (and they want to do so).