janet-lang / spork

Various Janet utility modules - the official "Contrib" library.
MIT License
122 stars 35 forks source link

Handle subcommands and positional arguments #66

Closed CFiggers closed 1 year ago

CFiggers commented 2 years ago

Previous functionality should be unaltered. All previous tests still pass.

Subcommands may precede or follow options marked by -f or --foo without issue. Args following -- still parse as expected.

bakpakin commented 2 years ago

Any particular reason we don't have a recursive definition for subcommands? For example, a situation where each subcommand has different sets of flags. I suppose an argument could be made for a slightly simpler interface but it seems like the more natural, obvious solution to me. I'm not set on either way, just curious if this was intentional or just easier.

Positional arguments with arity checking is obviously quite useful (as you have implemented), but it would be nice to have that in the main parser, not just in the subcommands - perhaps :min and :max arguments to :accumulate options?

CFiggers commented 2 years ago

Being able to define different sets of flags for each subcommand does make sense to me. That would help with avoiding collisions (for e.g. if -l matches a different long option in each subcommand, like --list vs --lookup in a different subcommand). I didn't go that far with it mostly because that's a much more extensive change—and also I'm not 100% sure what the best way to generate --help text for that would be.

:min and :max options make sense to me too—that's probably more generalizable than :args-expected and :args-required the way I have. :args-expected is essentially functioning like a :max already, since if it isn't also :args-required it only complains if you go over that number.

It occurs to me that we'd have to decide on a default behavior if the :min is set higher than the :max.

Techcable commented 2 years ago

Yeah I really think subcommands should be a recursive argparser within the main one.

This is more or less how Python click and Rust clap work (but more complex).

This would allow maximum flexibility and subcommands could be handled just like top-level commands. Only the parent parser would need special support (most importantly to generate docs).

Also I think we should support positional arguments as first class citizens. This should improve help generation.

:min and :max options make sense to me too

👍

Techcable commented 2 years ago

Positional arguments with arity checking is obviously quite useful (as you have implemented), but it would be nice to have that in the main parser, not just in the subcommands - perhaps :min and :max arguments to :accumulate options?

Ideally multiple positional arguments could be specified. This way they could have independent help messages.

tionis commented 1 year ago

What's the current state of the code in this PR? My question is mainly if the code is working or does it need some more work?

bakpakin commented 1 year ago

I haven't merged this since the arity checking code belongs in the main arg parser, and having the subcommand just be implemented more or less as recursion seems cleaner to me. Someone could convince me otherwise, though.

CFiggers commented 1 year ago

It is working code, at least as far as I know. But I wouldn't call it either elegant or well-designed. And without the improvements requested by @bakpakin I don't think I recommend it be merged. Since I don't expect to prioritize making those updates myself any time in the near future, I think I will go ahead and close this PR.