anntzer / defopt

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

How can we specify `None` for an optional argument with a default value #87

Closed nh13 closed 3 years ago

nh13 commented 3 years ago
from typing import Optional

def main(*, value: Optional[int] = 2) -> None:
    print(f"{value}")

if __name__ == '__main__':
    defopt.run(main)

I'd like to run python example.py -v None to set the value to None.

anntzer commented 3 years ago

What do you think should happen if the annotation was Optional[str] instead?

nh13 commented 3 years ago

Good question, my suggested behavior would be:

I've used of the "special argument value" :none: in Scala for optional values (of all types): https://github.com/fulcrumgenomics/sopt/tree/master#optional-arguments. I am not wedded to the actual value, just something that wont be commonly used.

anntzer commented 3 years ago

I didn't want to pick a representation for None, so I pushed another approach to the repo: you can now register your own parser for type(None) and that'll get picked up for parsing Optional too (as Optional[foo] is the same as Union[foo, type(None)]). (See the test for an example.)

How does that look to you?

nh13 commented 3 years ago

Just tested, this is great! Thank-you for the rapid response and implementation. I'll look forward to a minor release so we can use it!

nh13 commented 3 years ago

For others, this is the dummy example updated:

import defopt
from typing import Optional

def main(*, value: Optional[int] = 2) -> None:
    print(f"type: {type(value)} value: {value}")

def parser(string):
    if string == 'None':
        return None
    else:
        raise ValueError("Not None")

if __name__ == '__main__':
    defopt.run(main,parsers={type(None): parser})
$ python run.py 
type: <class 'int'> value: 2
$ python run.py -v 3
type: <class 'int'> value: 3
$ python run.py -v :blah:
usage: run.py [-h] [-v VALUE]
run.py: error: argument -v/--value: invalid typing.Optional[int] value: ':blah:'
$ python run.py -v None
type: <class 'NoneType'> value: None
anntzer commented 3 years ago

6.1.0 is released.

nh13 commented 3 years ago

@anntzer this is not working for when the type is Optional[Path] versus Optional[int]. I assume that's because Path is "list-like" and not supported in unions?

I do think there's value in differentiating empty collections (eg. []) from no collection (eg. None) for an optional collection type (eg. Optional[List[str]]), but I frequently use Optional[Path] as inputs for optional input/output arguments.

import defopt
from typing import Optional
from pathlib import Path

def parser(string):
    if string == 'None':
        return None
    else:
        raise ValueError("Not None")

def sub_path(*, value: Optional[Path] = None) -> None:
    print(f"path: {value} type: {type(value)}")

def sub_int(*, value: Optional[int] = None) -> None:
    print(f"path: {value} type: {type(value)}")

if __name__ == '__main__':
    defopt.run([sub_path, sub_int],parsers={type(None): parser})
$ conda env export | grep defopt
  - defopt=6.1.0=py37h89c1867_0
$ python run.py sub-int -v None
path: None type: <class 'NoneType'>
$ python run.py sub-path -v None
path: None type: <class 'pathlib.PosixPath'>
anntzer commented 3 years ago

I believe(?) that your interpretation is incorrect (defopt._is_list_like(Path) returns False); the behavior you report simply arises from the fact that Optional[X] is defined as Union[X, type(None)], that defopts tries Union parsers in the order they are given in the Union, and that Path("None") doesn't raise an exception?

The first solution I can think of is, I guess one could add a rule that defopt will check whether Unions contain "type(None)-or-Literals", and if so, check their parsers first (as they are more likely to be confused by later parsers, e.g. Path("None") here)? If so, implementing this is left as an exercise for you :-)

nh13 commented 3 years ago

That’s a great path forward and thank you for being open to it. I’ll be happy to make a PR once I get it working!

anntzer commented 3 years ago

This should now be fixed as of master.