RSDK-9284 - automate CLI flag parsing #4581
Open
+76
−7
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
viambot
added
the
safe to test
viambot
added
safe to test
stuqdog
commented
Nov 22, 2024
| camelFormattedName = matchAllCap.ReplaceAllString(camelFormattedName, "${1}-${2}") | ||
| camelFormattedName = strings.ToLower(camelFormattedName) | ||
|
|
||
| return ctx.Value(camelFormattedName) |
There was a problem hiding this comment.
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.
cli/app.go
Outdated
Comment on lines
267
to
275
| type foo struct { | ||
| FooFoo string | ||
| Bar int | ||
| } | ||
|
|
||
| func doFoo(foo foo, ctx *cli.Context) error { | ||
| fmt.Printf("FooFoo is %s and Bar is %v.", foo.FooFoo, foo.Bar) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
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
|
fyi @dgottlieb since we discussed this idea a bit during the scope doc process. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Actionfield 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.contextin thestructful methods that we're asking users to now define, as certain existing methods (DownloadModuleAction, e.g.) use fields on thectxwithin 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.