cli
cli copied to clipboard
Minimize top-level API surface area for both usability clarity and maintenance reduction
- [x] #1586
- [x] #1600
- [x] #1587
- [x] #1759
- [x] #1780
- [ ] #1791
(Continuing discussion from #833)
context.Context is explicitly not meant to be extended
@meatballhat any documentation or links to where that explicit convention maybe written?
@marwan-at-work I should have explained myself better, sorry! The interpretation of the go docs I'm leaning on here comes from the overview
Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it. The Context should be the first parameter, typically named ctx:
func DoSomething(ctx context.Context, arg Arg) error { // ... use ctx ... }
meaning that we're not supposed to be wrapping context.Context
with our own type because that goes against the convention of accepting context.Context
as first argument for the purpose of keeping the semantics/expectations of using context.Context
as simple as possible.
Aside from that particular interpretation issue, I'd like to make sure we are moving towards the design detailed in the series of issues tracked in #1531 which would result in an action func signature of:
type ActionFunc func(ctx context.Context, cmd *Command) error
@marwan-at-work can you move your comment over here? 🙇🏼
@meatballhat ah, yes storing context vs passing it. In most cases, you want a function to accept a context. But FWIW the Go team themselves break that rule in multiple places:
- http.Request stores its context as a private field and exposes
Context()
andWithContext
methods to get/set a request's context. - the sql package also stores its context for queries: https://github.com/golang/go/blob/d5de62df152baf4de6e9fe81933319b86fd95ae4/src/database/sql/sql.go#L2159
- The
os
package actually does exactly what we're doing here and embedding the context interface: https://github.com/golang/go/blob/d5de62df152baf4de6e9fe81933319b86fd95ae4/src/os/signal/signal.go#L299
With all of that said, I'm definitely happy with any solution as long as we have a full context accessible from the outside :)
From issue 1531, it looks like y'all are thinking of removing the *cli.Context
type completely? If so, then the type signature of (context.Context, *Command)
would also make sense.
@marwan-at-work Thank you for caring about this! 💖 Sorry for my (two weeks??) delay!
The places you reference in the standard library are great examples of how the core Go team adapted existing APIs after the context
library was introduced, IIUC. Thankfully, we don't have those constraints in github.com/urfave/cli/v3
🎉
I'm leaning more and more towards wanting to remove the *cli.Context
type completely, yep 👍🏼 with the goal being:
type ActionFunc func(context.Context, *Command) error