ipfs / go-ipfs-cmds

IPFS commands package
MIT License
50 stars 42 forks source link

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

Open mvdan opened 3 years ago

mvdan commented 3 years ago

A number of flag types are supported: https://github.com/ipfs/go-ipfs-cmds/blob/4ade007405e5d3befb14184290576c63cc43a6a3/option.go#L178-L198

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

welcome[bot] commented 3 years ago

Thank you for submitting your first issue to this repository! A maintainer will be here shortly to triage and review. In the meantime, please double-check that you have provided all the necessary information to make this process easy! Any information that can help save additional round trips is useful! We currently aim to give initial feedback within two business days. If this does not happen, feel free to leave a comment. Please keep an eye on how this issue will be labeled, as labels give an overview of priorities, assignments and additional actions requested by the maintainers:

Finally, remember to use https://discuss.ipfs.io if you just need general support.

mvdan commented 3 years ago

The linked PR above would also benefit from this :)

Stebalien commented 3 years ago

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 commented 3 years ago

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 https://github.com/djdv/go-filesystem-utils/pull/3/commits/a907d35c7a44e4f3575b76208c12025358e3cffb 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.