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

add support for custom flag types via flag.Value #213

Open
mvdan opened this issue Jul 21, 2021 · 4 comments
Open

add support for custom flag types via flag.Value #213

mvdan opened this issue Jul 21, 2021 · 4 comments
Labels
need/triage Needs initial labeling and prioritization

Comments

@mvdan
Copy link

mvdan commented Jul 21, 2021

A number of flag types are supported:

go-ipfs-cmds/option.go

Lines 178 to 198 in 4ade007

func BoolOption(names ...string) Option {
return NewOption(Bool, names...)
}
func IntOption(names ...string) Option {
return NewOption(Int, names...)
}
func UintOption(names ...string) Option {
return NewOption(Uint, names...)
}
func Int64Option(names ...string) Option {
return NewOption(Int64, names...)
}
func Uint64Option(names ...string) Option {
return NewOption(Uint64, names...)
}
func FloatOption(names ...string) Option {
return NewOption(Float, names...)
}
func StringOption(names ...string) Option {
return NewOption(String, names...)
}

This mostly mirrors what https://pkg.go.dev/flag supports, with one notable exception: flag.Value via https://pkg.go.dev/flag#Var.

This is very useful for custom types which know how to be used as flags. For example: https://pkg.go.dev/github.com/multiformats/go-multicodec@master#Code.Set

Right now, if I want a go-ipfs tool to have a multicodec.Code flag, I have to use StringOption, and then separately write about 4 lines of extra code to translate that via a Code.Set call. It would be much easier to be able to use it directly, via a go-ipfs-cmds API like VarOption or FlagValueOption.

Note that this does not require using the flag package for parsing arguments or flags. flag.Value is an interface, so it should be compatible with the current logic: https://pkg.go.dev/flag#Value

cc @willscott @masih

@mvdan mvdan added the need/triage Needs initial labeling and prioritization label Jul 21, 2021
@welcome

This comment has been minimized.

@mvdan
Copy link
Author

mvdan commented Jul 21, 2021

The linked PR above would also benefit from this :)

@Stebalien
Copy link
Member

I would love for this to happen, but, FYI, there are eldritch dragons here. If you can find a way to make this work with, e.g., our Files arguments, stdin support, and all of our other... interesting features, go ahead!

But I figured I'd warn you. At the very lease, please try to avoid major refactors because I've reviewed at least 3 refactors here and all of them have been... difficult (race conditions, strange edge cases, etc).

@djdv
Copy link
Contributor

djdv commented Jul 23, 2021

I don't know if this is helpful or not but I ended up writing a small library specifically to skirt around this.
Just providing it in case it's interesting/relevant, but it might not be. Feel free to ignore.

In particular I wanted to be able to add a field whatever multiaddr.Multiaddr to a "settings struct" and have it automatically added to the cmds lib portions of the program.
Both generating the cmds.Option as well as parsing it. Special rules for complex types are moved into that lib rather than into the cmds.Command.Run section so everything is uniform.
I also needed inheritance and other things to make it easier to do multiplatform things automatically, but this is farther from the topic.

Screencast of the initial phase: https://www.youtube.com/watch?v=6oGsKnfZFzA and slightly later https://www.youtube.com/watch?v=7LI0_EequHA
The WIP hasn't been split out yet but a commit using it is here djdv/go-filesystem-utils@a907d35
Edit: still WIP but this tree this tree is slightly better that the commit above. Particularly the tests have some documentation.
* I should also mention this is a gross abuse of reflection and runtime tricks. But I find using it convenient.

I'm planning to extend this soon so that a config file can be used as a source and I won't have to change anything in any of the the cmds.Command.Run logic except adding the config as an argument source in addition to command line and environment values. Then basically for free all the command will automatically be aware of values from a file too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/triage Needs initial labeling and prioritization
Projects
None yet
Development

No branches or pull requests

3 participants