structs should default to "embed" behaviour if "cmd" isn't specified.

[nergdron]

just getting going with kong, liking it a lot more than other cli parsers I've used. however, there's one minor feature request I'd love. If I've got some nested structs, and the internat struct is tagged with neither "embed" or "cmd", it'd be great if it defaults to the "embed" behaviour, rather than throwing an error:

type sample struct {
  Inline struct {
    New bool `default:"true"`
  }
}

func main() {
  s := new(sample)
  kong.Parse(s)
  log.Println(s)
}

without any tags on Inline:

panic: unsupported field type main.sample.Inline (of type struct { New bool "default:\"true\" negetable" }), perhaps missing a cmd:"" tag?

goroutine 1 [running]:
github.com/alecthomas/kong.Parse(0x566380, 0xc0000c00a0, 0x0, 0x0, 0x0, 0x0)
	/home/tessa/code/go/pkg/mod/github.com/alecthomas/kong@v0.2.16/global.go:11 +0x145
main.main()
	/home/tessa/code/mine/cnafu/example/main.go:21 +0x5c
exit status 2

with the embed tag:

2021/05/08 19:44:24 &{{true}}

I don't see the value in having an untagged embedded struct cause a runtime panic, I think it'd be more intuitive if embed was the default.

[alecthomas]

I believe it's better to be explicit. For example while you think embed should be the default I would intuitively expect cmd would be, so
somebody is going to be surprised.

[nergdron]

well I guess to me, you need more configuration for cmd to work? whereas embed will give you useful behaviour without doing anything else. so the one that matches "no configuration" is the one that should be the default. "requires extra code" should always be the optional behaviour, I think.

I guess if you think that the library should just give up on missing tags, I think panicking is still the wrong response. would you be opposed to updating some of these functions to return errors rather than just crash? it'd be a lot nicer to deal with than the current behaviour I'm seeing.

[alecthomas]

You do not need more configuration for cmd to work - the effort required is identical. The only difference is that cmd will create a sub-command named after the field.

As a developer error (as opposed to a user error), panicking rather than erroring is deliberate. What would be the benefit to returning an error?

[nergdron]

well, I mean, you have to actually implement a sub-command when using cmd. with embed, it just gives me the convenience and organization of structured config data. cmd dictates the structure of my program, while embed lets me use my config struct as-is with the existing structure.

as for the panic case... well, incorrectly parsed commandline args do not indicate a fatal error case in most of my apps, as there's plenty of other sources of configuration (files, env vars, sensible defaults, config stores, etc). it's certainly an error that should be handled, but the point of a panic (and the way in which it's handled differently from functions that returns errors) is centered around the idea that the error is fatal to program execution. a misplaced struct tag (especially with structs that are built through reflect from other sources) shouldn't constitute a fatal error, IMO.

certainly it's a lot easier to use standard go patterns for dealing with a returned error defined by the library than it is to create a deferred recovery function and backtrack through your entire lib to see where the panic happened, and if it's a real problem that actually should hald to program or if it's just a missing tag. 🤷‍♀️

in any case, it sounds like you prefer the behaviour of the lib always treating parsing errors as fatal, so it sounds like it isn't a good fit for me. thanks anyway!

[mickael-menu]

in any case, it sounds like you prefer the behaviour of the lib always treating parsing errors as fatal, so it sounds like it isn't a good fit for me. thanks anyway!

If your code is incorrect, crashing the program seems appropriate. Kong doesn't crash if the user enters an invalid command, which is expected.

End users should never see these panics, unless you are shipping without testing your app.

[nergdron]

@mickael-menu I probably wasn't totally clear from my last comment, but I'm synthesizing config structures from sources outside go, as should be indicated from the comments about other config sources and reflect. so the structs being parsed are in fact user content, not static content generated before compile time. I realize this is an unusual use case, but yes, my code gets fully tested, and no, I can't account for all config structs that may be used ahead of time. by necessity, my code needs to gracefully handle loading data into structs that are unknown, and likely not correctly tagged, at least not initially.

now, you may not want to handle meta-programming use cases, and that's fine. it's just the problem space I'm working in and so kong evidently isn't a good fit. I'm still not convinced "panic is just as good as regular errors", but ultimately it's the behaviour y'all want for your lib, and you seem very firm on that stance. so I'll move on, no worries.

[mickael-menu]

Sorry, I misunderstood your use case.

I don't know if it would help in your situation, but Kong now supports registering dynamic commands:

kong/options.go

Line 67 in 49417fe

func DynamicCommand(name, help, group string, cmd interface{}) Option {

[nergdron]

hmmm. that actually might be helpful, yeah. that combined with using reflect to inject embed in stuff that isn't supposed to be a subcommand would get me some of the way there, for sure. I think I'll still have to be pretty careful of panics, but I'll have to check through the code and see if there's any in my used code paths after using DynamicCommand. thanks for the pointer!

[alecthomas]

panic is just as good as regular errors

This is not accurate at all. Kong uses panic to convey programmer errors. That is normal, idiomatic Go.

It sounds like your use case isn't a great fit as you're using Kong in a way it wasn't designed for. Best of luck.