cli icon indicating copy to clipboard operation
cli copied to clipboard

Minimize top-level API surface area for both usability clarity and maintenance reduction

Open Dokiys opened this issue 1 year ago • 4 comments

  • [x] #1586
  • [x] #1600
  • [x] #1587
  • [x] #1759
  • [x] #1780
  • [ ] #1791

Dokiys avatar Oct 13 '22 10:10 Dokiys

(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

meatballhat avatar Nov 27 '22 14:11 meatballhat

@marwan-at-work can you move your comment over here? 🙇🏼

meatballhat avatar Nov 27 '22 18:11 meatballhat

@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:

  1. http.Request stores its context as a private field and exposes Context() and WithContext methods to get/set a request's context.
  2. the sql package also stores its context for queries: https://github.com/golang/go/blob/d5de62df152baf4de6e9fe81933319b86fd95ae4/src/database/sql/sql.go#L2159
  3. 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 avatar Nov 28 '22 16:11 marwan-at-work

@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

meatballhat avatar Dec 11 '22 14:12 meatballhat