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

Why reimplement flag parsing in this package? #79

Open
dignifiedquire opened this issue Mar 17, 2018 · 3 comments
Open

Why reimplement flag parsing in this package? #79

dignifiedquire opened this issue Mar 17, 2018 · 3 comments
Assignees

Comments

@dignifiedquire
Copy link
Member

Both
-https://golang.org/pkg/flag/ and

seem like good and battle tested options to parse commandline flags, why is this library reimplementing all of that?

@keks
Copy link
Contributor

keks commented Apr 9, 2018

Parsing has been in here back when this package still was go-ipfs/commands and one reason is that this part simply was not revised yet. The standard library's flag package would definitely break backwards CLI compatibilty (it doesn't read flags after non-flag args), maybe spf13's pflag would work, though.
I'll investigate.

@keks keks self-assigned this Apr 9, 2018
@keks
Copy link
Contributor

keks commented Apr 9, 2018

I think the main issue is that we not only parse options and arguments, but also the command path, and that each command comes with different options and arguments. In both "flag" and "githhub.com/spf13/pflag" you need to specify the options a priori.

What we could do is first parse the command line with the flag parser only knowing about the root command options, use the first argument as command, load that commands options into the flag parser and start again. Repeat that until the entire command line is processed or we hit an undefined argument or option.

One problem I see is getting the error messages look exactly like they do now, but I think we'll need to just go ahead and start implementing to see if that ever happens - but maybe I'm taking backwards compatibily a bit too serious here and it's not a problem, don't know.

@keks
Copy link
Contributor

keks commented Apr 9, 2018

Another issue I'm hitting is that we're take a slice of alternate option names, but pflag only uses one long and one short (single char) flag. Unfortunately, in some cases we use several multi-char short names:

ipns.go:62:		cmdkit.UintOption("dht-record-count", "dhtrc", "Number of records to request for DHT resolution."),
ipns.go:63:		cmdkit.StringOption("dht-timeout", "dhtt", "Max time to collect values during DHT resolution eg \"30s\". Pass 0 for no timeout."),
resolve.go:68:		cmdkit.UintOption("dht-record-count", "dhtrc", "Number of records to request for DHT resolution."),
resolve.go:69:		cmdkit.StringOption("dht-timeout", "dhtt", "Max time to collect values during DHT resolution eg \"30s\". Pass 0 for no timeout."),

I think this is from from the ipns-via-dht PR. Is that still experimental? In that case we could still change the short flags so they are single-character. That would definitely be cleaner than search-and-replacing elements like "-dhtt" in the command line string before giving it to the argument parser (though that might be a fallback solution).

cc @Stebalien

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

No branches or pull requests

2 participants