feat/cli: migrate version subcommand to use urface/cli#1292
feat/cli: migrate version subcommand to use urface/cli#1292
Conversation
keegancsmith
left a comment
keegancsmith
left a comment
There was a problem hiding this comment.
I'm kinda confused how things worked before for subcommands and flags... but yeah if your testing works (especially trying out cli flags for the subcommand), then lets do it.
Is there a wayt o migrate future commands which don't break them up so much / adjust indentation so much. That would make the PRs much easier to validate.
| args := flagSet.Args()[1:] | ||
| if err := cmd.flagSet.Parse(args); err != nil { |
There was a problem hiding this comment.
I'm kinda confused how this all worked before. .Args is the non flag arguments, not the raw argv.
cmd/src/cmd.go
Outdated
| for _, cmd := range c { | ||
| if !cmd.matches(name) { | ||
| _, isMigratedCmd := MigratedCommands[name] | ||
| if !isMigratedCmd && !cmd.matches(name) { |
There was a problem hiding this comment.
this is kinda funky to read and will also break when one day c is empty.
you don't do that much inside of this for loop. Why don't you pull out a check for MigratedCommands before this for loop and if you have a match in it, just have a simpler bit of code to run that command (with readConfig/etc duplicated)
There was a problem hiding this comment.
Agreed! I'll move it
There was a problem hiding this comment.
Another reason to move it, I just realised it has this issue with how its currently implemented. Any subcommand can be a global migrated command:
❯ go run ./cmd/src auth version
Current version: dev
Recommended version: 7.0.3 or later
There was a problem hiding this comment.
Well now that is awkward 🤣 Good spot! Push the fix in a few
There was a problem hiding this comment.
Thought I report back on what is happening here :D A migrated command becomes global by "accident", that is because the run we execute here, gets executed again on a child/subcommand again.
For src auth version
-- first pass ('src', 'auth', 'version') --
> subcommand is `auth`
> auth is not a migrated command and `runLegacy` gets executed
> In `runLegacy`, cmd.run (auth.run) gets called - which is the same run loop we just executed ...
-- second pass ('auth', 'version') --
> subcommand is `version`
> version is a migrated command and `runMigrated` gets executed
So for the fix, I will be moving runMigrated completely out of commander.run
|
@burmudar I realised how it works now and came back to comment why. golangs flagset packages will treat all arguments after the first non flag as arguments. So |
burmudar
force-pushed
the
wb/urfave-cli-init
branch
from
d6f448a to
e80aa76
Compare
burmudar
force-pushed
the
wb/urfave-cli-init
branch
from
e80aa76 to
349b951
Compare
3 participants
This introduces a compatibility layer to start migrating src-cli to use a cli framework.
Some of the benefits we get by using a cli framework:
If a command is within the migrated set, we then defer to executing the command within urfave/cli with
runMigrated. If the command is not in the set we executing the legacy code withrunLegacy.Test plan
Tested locally