RSDK-9284 - automate CLI flag parsing

[stuqdog]

Note to reviewers: this is currently a POC and definitely not ready for prime time. Before I go ahead and make changes to all existing methods, I wanted to get buy-in from folks on this as an approach. Once we have agreement on the shape of this implementation, I'll do the (verbose, but mechanical) work of changing existing methods which should hopefully be trivial to review despite having an estimated large loc diff.

The nice thing about this approach is it provides safe, easy, typeful access to flag data while requiring minimal change in how developers create actions (they have to define a struct with their flag fields and the Action field is now populated slightly differently), but there should be an easily replicable pattern in the code base such that this is not harmful.

A notable shortcoming of this approach is that the names of the fields in the struct must match (or fuzzy match, taking account for differences in snake/camel/kebab case) the names of the flag. Since a developer is defining the struct, we don't currently have a way to enforce that things are correct. I do think it would be possible using reflection to have some sort of assert that the flags and the fields of the struct fuzzy match, but I fear this will require some decent refactoring and is more of a "programmatic enforcement" issue rather than a "automate flag parsing" issue and so should be done in a future PR.

One other (minor) restriction: the fields on the struct must be public. The reflection library can't access them if they're private, leading to a runtime error.

One other thing to note: I don't think we can get away with not requiring a CLI.context in the structful methods that we're asking users to now define, as certain existing methods (DownloadModuleAction, e.g.) use fields on the ctx within the method. This means that if a user wants to access flags the old way, we can't stop them. Again, this might be resolvable but probably should go in a separate ticket.

[stuqdog]

It seems like some guardrails might be nice here, but I'm not sure that we can say at compile time what's safe and what's not. Even if an argument is non-optional, I expect there are cases where a nil value is normal and expected.

[stuqdog]

This will get deleted in the final project obviously, but I wanted to show an example of what new development would look like. Also, it highlights how we successfully set a field FooFoo with a flag of foo-foo

[stuqdog]

fyi @dgottlieb since we discussed this idea a bit during the scope doc process.

[benjirewis]

cc @zaporter-work

[zaporter-work]

This seems like a good improvement -- I like the flags object (hopefully people will catch name-mismatches manually). However, If you're interested in other options, I've enjoyed this pattern for some of my personal projects. It takes a bit of naming-awareness, but it is fairly simple. This uses urfavecli/v3 instead of v2 (though it might be compatible with v2):
main.go:

func main() {
	cmd := &cli.Command{
		Name: "sudoku",
		Commands: []*cli.Command{
			createStatsCli(),
			createDataCli(),
			createGraphCli(),
			createEvaluateCli(),
			createExperimentCli(),
			lambda.CreateLambdaCli(),
		},
	}
	if err := cmd.Run(context.Background(), os.Args); err != nil {
		log.Fatalln("error:", err)
	}
}

example stat.go

func createStatsCli() *cli.Command {
	graphPath := ""
        printUnfinishedSteps := false
	action := func(_ context.Context, _ *cli.Command) error {
                // SOME VERY SHORT FUNCTION
                // (that uses the vars in the parent scope)
		graph, err := LoadGraphFromFile(graphPath)
		if err != nil {
			return err
		}

		var pStats []ProblemStats
		for _, p := range graph.Problems {
			pStats = append(pStats, p.GenStats(graph))
		}
		PrintStatsInfo(pStats, printUnfinishedSteps)
		return nil
	}

	return &cli.Command{
		Name: "stats",
		Arguments: []cli.Argument{
			&cli.StringArg{
				Name:        "graph",
				Destination: &graphPath,
			},
		},
		Flags: []cli.Flag{
			&cli.BoolFlag{
				Name:        "print-unfinished-steps",
				Destination: &printUnfinishedSteps,
			},
		},
		Action: action,
                // can support nested Commands via
               // Commands: []*cli.Command{moreCreateInvocations()}
	}
}

This has the benefit of not using runtime reflection -- though it does scatter a lot of the cli tree out into different functions instead of one giant object. I like it for personal projects because the Destination: &var means I don't have to worry about any magic strings in contexts. (it also makes it easy to use Arguments instead of just Flags)

---

No pressure to use this though! Just wanted to throw something out there to consider while you restructure the flag parsing.

[benjirewis]

Thanks for thinking about this + putting a PoC together @stuqdog! I really like how this method does not massively change how we're adding new pieces to the highest level cli.App struct. I do have three issues with it:

1. You are storing values on contexts
   a. I can totally get over this one, but it is generally an anti-pattern in Go in the sense that it should be a "last-resort" strategy (this might be a "last-resort" case)
2. You are using runtime reflection
   a. Your reflection code (getValFromContext) will run for each command at each invocation. Putting reflection in "hot paths" is usually a bad idea because reflection is relatively slow
3. Your fuzzy searching allows multiple ways of specifying a flag
   a. Most CLIs I have interacted with only allow one or two ways of specifying a flag (usually -c and -count or something similar) and are pretty stringent about using hyphens between words instead of underscores. I think users will be surprised by the flexibility here especially if it is undocumented

I like the idea of @zaporter-work's solution, but it looks like we are expecting developers to use the Destination pattern (which is a good one!) in their create*CLI functions. If they forget to do that (which I think they will, honestly,) then all we've done is, as Zack mentioned, obfuscate the definition of the top-level cli.App.

I realize I am only criticizing and not offering other solutions 😮‍💨 ; let me take a stab at something locally and get back to you guys. If we really want to get Viam developers to add ActionFuncs in a more consistent manner, I think we'll have to design a new API for adding a Command, and a custom struct that wraps a *cli.App.

[stuqdog]

Thanks for thinking about this + putting a PoC together @stuqdog! I really like how this method does not massively change how we're adding new pieces to the highest level cli.App struct. I do have three issues with it:

1. You are storing values on contexts
   a. I can totally get over this one, but it is generally an anti-pattern in Go in the sense that it     should be a "last-resort" strategy (this might be a "last-resort" case)

2. You are using runtime reflection
   a. Your reflection code (`getValFromContext`) will run for each command at each invocation. Putting reflection in "hot paths" is usually a bad idea because reflection is relatively slow

3. Your fuzzy searching allows multiple ways of specifying a flag
   a. Most CLIs I have interacted with only allow one or two ways of specifying a flag (usually `-c` and `-count` or something similar) and are pretty stringent about using hyphens between words instead of underscores. I think users will be surprised by the flexibility here especially if it is undocumented

I like the idea of @zaporter-work's solution, but it looks like we are expecting developers to use the Destination pattern (which is a good one!) in their create*CLI functions. If they forget to do that (which I think they will, honestly,) then all we've done is, as Zack mentioned, obfuscate the definition of the top-level cli.App.

I realize I am only criticizing and not offering other solutions 😮‍💨 ; let me take a stab at something locally and get back to you guys. If we really want to get Viam developers to add ActionFuncs in a more consistent manner, I think we'll have to design a new API for adding a Command, and a custom struct that wraps a *cli.App.

Hey! I think there's some slight misunderstanding specifically around points 1 and 3, messaged you offline to chat about it :)

[stuqdog]

If they forget to do that (which I think they will, honestly,) then all we've done is, as Zack mentioned, obfuscate the definition of the top-level cli.App.

Yeah I like @zaporter-work's suggestion a lot but I do worry it expects too much of developers in terms of remembering the right way to do things.

@benjirewis and I are speaking this afternoon and will come up with some thoughts on how to proceed from there!

[benjirewis]

@stuqdog and I spoke offline, and he assuaged all three of my previous concerns. I cannot come up with some clever way to do this without any runtime reflection, but we both agree that the performance hit of using reflection here is fine given the usage of this code in CLI (user is almost always entering one command and waiting for a response.) We did want to create a ticket for a subsequent PR that wraps the cli.App in a custom ViamCLIApp struct that introduces a bit more type safety around adding new commands.

I do have a couple nits and one question about your handling of types beyond string/optional types.

[benjirewis]

[nit] Could you add comments above each of these regexes giving an example of a pattern they would match? I find that to be extremely helpful when reading through code and seeing (well-named but otherwise) random regular expressions.

[benjirewis]

Suggested change

	func getValFromContext(name string, ctx *cli.Context) interface{} {
	func getValFromContext(name string, ctx *cli.Context) any {

[nit] Prefer any to interface{} post Golang 1.18 (here and elsewhere.)

[benjirewis]

Hopefully all Golang variable names are camelCased (lint would likely complain,) but thanks for defensiveness here.

[benjirewis]

I'm surprised you can have int fields here. I thought the value stored on the cli.Context would be a string. How do you not get a runtime error on L260 above when you try to set the Bar int value to a string? Similarly, if a user does not specify -bar flag, would we not try to set the Bar int value to nil?