- Notifications
You must be signed in to change notification settings - Fork18
Migrate to cobra#86
Uh oh!
There was an error while loading.Please reload this page.
Conversation
cmd/coder/secrets.go Outdated
} | ||
} | ||
type listSecretsCmd struct{} | ||
func makeCreateSecret() cli.Command { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Idk how I feel about this.
b9cb39d
to316033e
CompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Do you prefer v2 or v1 urfave@cmoog? |
From looking that the migration docs it just seems like they made envs accept a slice and made most of the command/flag structs pointers instead. |
It looks like they enforce that flags are positioned before arguments too. Definetly not a fan of that. For example, the following is invalid.
In v2, this must be
|
Yea that straight sucks :( |
cmoog commentedAug 6, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
|
I’d be in favour of using cobra then. |
Nevermind, I think the flag-before-arg is fine given that the |
From a UX perspective it's super annoying imo when I'm editing a CLI command. Any context on why they made this change? |
@nhooyrhttps://github.com/urfave/cli/blob/master/docs/migrate-v1-to-v2.md#flags-before-args
|
That’s so dumb. What does it have to do with POSIX lol. |
If you're ok migrating, I'd say we go for cobra instead for UX. But I defer to you. |
There are some error handling inconsistencies but nothing too bad that can't be fixed after this merges. Don't want to keep this unmerged much longer as it's blocking feature work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
how's the fish completion now?
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
@@ -0,0 +1,26 @@ | |||
# ci |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Would be good to link to this from the README.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Most readers of theREADME
will be customers, not developers. Doesn't really make sense to bring attention to it in my view.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Don't have to bring attention to it. Just put a link at the bottom. Customers/potential customers who care to look closely will be happy we document and care about these things.
Uh oh!
There was an error while loading.Please reload this page.
I'm a little swamped right now, don't think I can give this a good review sorry! |
To unblock feature work and prevent a rush before the release date, I'm going to merge this now. Some cleanup PRs will be following in parallel. |
Uh oh!
There was an error while loading.Please reload this page.
I plan on following this up immediately with a cleanup of the error handling across the CLI layer. I'd prefer to merge as soon as posable to prevent blocking additional work. We should move away from callingDone.flog.Fatal
and towards the built-in error handling present in theurfave/cli
.As noted by@nhooyr, the closure pattern is non-obvious. Let me know what you think.
This PR also includes many improvements/additions, most notably to the
envs
andurls
subcommands.urls
was kept at the top level to prevent unnecessary coupling, but I'm thinking we should move it to a subcommand ofenvs
.Closes#85
cc@nhooyr@anthonyshull@coadler
Closes#82