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

Include arument type infomation in option class, closes issue #3 #4

Closed
wants to merge 3 commits into from

Conversation

con-f-use
Copy link

@con-f-use con-f-use commented Aug 11, 2019

This PR makes argument validation easier by parsing argument types and making them accessible, if provided in the docstring.

E.g. merging this Pull request makes this possible:

"""Some stuff

Usage:
    {__package__} [options]

Options:
    -s --speed=<float>    Speed in km/h [default: 42]
    --count=<posint>      How often [default: 1]

"""
__doc__ = __doc__.format(**globals())


from docopt import docopt, parse_defaults


def convert_arg(atype, value):
    if atype == '<float>':
        return float(value)
    elif atype == '<posint>':
        value = int(value)
        if value < 0:
            raise ValueError('Must be positive integer!')
        return value
    return value


if __name__ == '__main__':
    args = docopt(__doc__, version="Dei mama hat a version!")
    atypes = {(o.longer or o.short):o.atype for o in parse_defaults(__doc__)}

    for key, value in args.items():
        if key in atypes:
            args[key] = convert_arg(atypes[key], value)

    print(args)

In action:

┌─11-18:15[con-f-use@confusion6:~/devel/docopt-ng] $ 
└▪ python playground.py
{'--count': 1,
    '--speed': 42.0}
┌─11-18:16[con-f-use@confusion6:~/devel/docopt-ng] $ 
└▪ python playground.py --count -1
Traceback (most recent call last):
    File "playground.py", line 35, in <module>
    args[key] = convert_arg(atypes[key], value)
    File "playground.py", line 24, in convert_arg
    raise ValueError('Must be positive integer!')
ValueError: Must be positive integer!

Note how --count and --speed are actual number types as opposed to strings.

see: #3

@con-f-use con-f-use changed the title Include arument type infomation in option class, closes #3 Include arument type infomation in option class, closes issue #3 Aug 11, 2019
@con-f-use
Copy link
Author

@itdaniher did this project go the way of original docopt?

@cjwelborn
Copy link
Contributor

cjwelborn commented Sep 4, 2019

@con-f-use, Do the types have to be strings? I haven't looked at this, but could we do if isinstance(atype, float) instead? That opens up a whole lot of possibilities of we have access to the actual class, instead of just a string with the class name in it. Just a thought, correct me if I'm way off base.

For instance, easy conversions become value = atype(value), wrapped in a try-block to catch exceptions.

Edit: Nevermind, I see you're just parsing the string and returning whatever the "name" for the value is. I can still get at the class if I need to.

@con-f-use
Copy link
Author

con-f-use commented Sep 4, 2019

Well, the way that arguments are specified in the doc string these "types" are arbitrary strings and do not have to match an existing (type-sub-)class from Python. There is no intrinsic type information or input validation with pure docopt. I.e. the user specifies them in the docstring like this:

Docstring.

Options:
    -s --speed=<float>    Speed in km/h [default: 42]

but they could just as well write:

Options:
    -s --speed=<wooseldoosel>    Speed in km/h [default: 42]

without docopt complaining. Of course you could write your docstrings in a way that these "type"-strings match existing types and do easy, automatic conversion. In fact that is one usecase for this PR.

I find this information useful by itself and my pull request wants to make it more accessible.

It would be pretty trivial to look for existing class and substitute the string value for the actual class if one is found. Not the scope of this PR, though, and docopt is more flexible if they remain strings because users can write additional custom code for that (the way my example shows) if they need it.

You might also want to look at the validation example and the data validation section in the readme of original docopt.

@cjwelborn
Copy link
Contributor

Yeh, after I read the actual PR I saw that it was just part of the doc string. I spoke too soon. I think itdaniher was talking about merging this in issue #4. I'm looking forward to it.

@NickCrews
Copy link
Contributor

Hey there @con-f-use @cjwelborn, I've picked this project up as a new maintainer.

At this point, I don't feel comfortable merging this. Sorry that you've both in in some effort here, but here are my reasons:

  • I'd like to see some other people express interest in this. If you like this feature request, give the original post a thumbs up
  • This is drifting away from the original spec outlined at docopt.org. Ideally I'd like this fork, since it has been picked up by jazzband, to become the canonical version, so I want to be judicious with what features we add.
  • At first glance (I haven't tried I'll admit) this seems to be not too difficult to implement yourself.

Do those concerns make sense?

@con-f-use
Copy link
Author

con-f-use commented May 31, 2022

It's been so long that I worked on this, so I might be wrong here, but...

This is drifting away from the original spec outlined at docopt.org. Ideally I'd like this fork, since it has been picked up by jazzband, to become the canonical version, so I want to be judicious with what features we add.

Not really it doesn't change any existing functionality, just gives an easier way to get at the specifics of the docstring. Nothing in the spec needs to change.

At first glance (I haven't tried I'll admit) this seems to be not too difficult to implement yourself.

If you mean, the user could get at the type stuff in the docstring themselves, then yes, but it is very awkward and requires a lot of code by comparison. It's much easier to have it built in this way. It's just information that is already there made available to the user a bit more readily and would be hard and lengthy to implement and likely involve monkey-patching docopt-ng or duplicating a good part of its docstring parsing and is error-prone.

Your first concern makes very much sense though. If people don't want it, who am I to argue. It's just that I would have had a couple of use cases and people wouldn't even know it there until they specifically look for that feature.

@NickCrews NickCrews force-pushed the master branch 2 times, most recently from ad9cb3f to ce4e236 Compare May 31, 2022 19:34
@con-f-use
Copy link
Author

@NickCrews Did you have a chance to read my reply just above an think it over? I'd really like to have this and most concerns of your's seem to be addressed.

@NickCrews
Copy link
Contributor

NickCrews commented Jul 21, 2022

Not really it doesn't change any existing functionality

You're right, it doesn't break anything so far, but it does diverge from the spec. I'm sorry, but I don't feel comfortable doing that. A community of 7.6k people who starred the original docopt expect the implementation to act a certain way, so I don't want to act unilaterally as a single person to change that implementation. I'm trying to build consensus around this fork and make it canonical, and and unstable and inconsistent API takes away credibility.

If other people express interest then we can revisit.

Here is a very short solution that works currently:

"""Some stuff

Usage:
    {__package__} [options]

Options:
    -s --speed    Speed in km/h (float) [default: 42]
    --count          How often (positive int) [default: 1]

"""python
__doc__ = __doc__.format(**globals())


from docopt import docopt


def to_pos_int(value):
    value = int(value)
    if value < 0:
        raise ValueError('Must be positive integer')
    return value


option_type_converters = {
    "--speed": float,
    "--count": to_pos_int,
}


if __name__ == '__main__':
    args = docopt(__doc__, version="Dei mama hat a version!")

    converted = {}
    for key, value in args.items():
        converter = option_type_converters.get(key, False)
        if converter:
            value = converter(value)
        converted[key] = value

    print(converted)

I don't think the rest of docopt supports making CLI interfaces that are so complicated that this method would ever get untenable. Do you have a complicated need where the above method doesn't work?

@con-f-use
Copy link
Author

con-f-use commented Jul 21, 2022

I can live with that, just consider two things:

  1. The problem with your snipped is, that the information is duplicated and can get out of sync. Also docopt's whole stick is, that the entire command line interface is specified in the docstring, hence self-documenting. I'm aware of many ways once can do data validation, the original docopt mentions schema, but this is more of an introspection and parsing feature that also happens to be good for validation and doesn't duplicate info.

image

  1. Not sure, but you seem to think that the --long-opt=<type> notation is not part of the spec. It is, however, mentioned in many places in the original docopt documentaiton. Also, it is part of POSIX and docopt claims to follow POSIX.

image

Sorry for being so persistent, I promise this is the last time I will be pestering you about it.

@h4l
Copy link
Contributor

h4l commented Aug 19, 2022

FWIW, as a docopt user, I value that docopt itself is super-simple. I get that you'd like to keep validation rules in-sync with the usage, but ultimately the usage text isn't rich enough to encode realistic validation rules in the general case. Sure, you can say something's a float, but as soon as it has to be in a certain range, or be constrained by the value of another option you're going to have to define extra validation rules outside the usage text.

IMO it's better to just let docopt stay focussed on parsing the arguments, and handle validation as a second step, using whatever method seems best. Personally I've had good results from using Pydantic to define a model of my arguments, and using it to validate the dict that docopt creates. That also gives you a sensible type to pass around.

@con-f-use
Copy link
Author

con-f-use commented Aug 19, 2022

This PR does not implement any validation, it just gives easy structured access to the information that's already in the docstring.

@h4l
Copy link
Contributor

h4l commented Aug 19, 2022

Sure. In my reply I was thinking of what you said here:

The problem with your snipped is, that the information is duplicated and can get out of sync.

Unless I'm misunderstanding, what you're providing with atype is the second part of an option, e.g. with --foo=<bar>, atype is the string "<bar>".

My feeling is that having access to atype can't actually solve this problem of duplicated/out-of-sync information. Because just knowing the atype of a value isn't enough by itself to validate it in the general case, we'll always need additional context which can't be encoded in the usage string.

For example:

usage: random_number [--min=<int>] [--max=<int>]

Knowing that a atype is <int> can't prevent the --min value from being greater than the --max value. So to validate this I'd need to have something defined outside the usage string which knows about the relationship between these values, hence something that can get out of sync with the usage string, regardless of having atype or not.

Also, I'm not sure that atype is the right name for what this provides. It's something like the name of an option's value, not necessarily a type. e.g. --from=<start> --to=<end>.

@NickCrews
Copy link
Contributor

NickCrews commented Aug 20, 2022

Not sure, but you seem to think that the --long-opt= notation is not part of the spec

Sorry, I wasn't super precise here upon re-reading what I wrote. You're right in the sense that this feature is part of the spec in the sense that it would not violate it. I mean it's not part of the spec in the sense that it isn't a documented feature. If something is supported, then I believe that it should be fully documented. If something isn't documented, then I don't think it should exist. I don't like secret features that are there for some people to discover. It makes things confusing to the users, if this project works differently than what docopt.org says. It would be hard to change docopt.org because we would need to build consensus between all the languages on what they want, then get write access from the current owner.

the usage text isn't rich enough to encode realistic validation rules in the general case

These are some great points, but I think this is the really big sticking point for me.

This PR does not implement any validation, it just gives easy structured access to the information that's already in the docstring

I don't get what having this information gains you? You're not saving any indirection anywhere, you either are going to have to have the mapping of "<posint>" -> to_pos_int (what this PR allows), or you have the mapping "--speed" -> to_pos_int (what we have right now, and in fact I think I prefer this one, it's more explicit).

I'm still not going to merge this at this point, but will leave it open.

@bittner
Copy link
Member

bittner commented Sep 28, 2022

Just as a heads-up, I'm interested in this feature (as in: display default values and the expected type). I'd also like to see type validation and having a choices list type.

This is drifting away from the original spec outlined at docopt.org.

@NickCrews Can you elaborate what is the "original spec" and why adding types and default values make us drift away from it?

In the meantime, I'm going to evaluate type-docopt, which seems to do it all, but I'm not sure yet whether that works with docopt-ng.

@NickCrews
Copy link
Contributor

NickCrews commented Sep 29, 2022

@NickCrews Can you elaborate what is the "original spec" and why adding types and default values make us drift away from it?

Hi! Good to hear that someone else wants this. Please, if you want this as well, keep chiming in, it is evidence that leads me to reconsider.

By "original spec" I mean the one published at docopt.org, which has become canonical as there are many other language forks that have implemented that. By "drift away", I meant same as what I said above:

it's not part of the spec in the sense that it isn't a documented feature. If something is supported, then I believe that it should be fully documented. If something isn't documented, then I don't think it should exist. I don't like secret features that are there for some people to discover. It makes things confusing to the users, if this project works differently than what docopt.org says. It would be hard to change docopt.org because we would need to build consensus between all the languages on what they want, then get write access from the current owner.

Does that make sense? Think I'm being silly?

It looks like you might be considering beginning to use docopt. If so, honestly, I might consider other more modern CLI tools, like Typer which at this point I think might be a better choice for new projects and I think it supports what you're looking for here.

@NickCrews
Copy link
Contributor

I'm going to close this as wontfix, because of

  1. inability to do complete validation.
  2. drift away from the original spec, which would cause confusion.

I'm sorry and I understand if people find this frustrating. Any new CLI work should be using a more modern CLI library I think, I just want docopt to stick around enough that people's ancient scripts will continue to work.

@NickCrews NickCrews closed this May 30, 2023
This was referenced May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants