Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
6 tasks done
Tracked by #833
Dokiys opened this issue Oct 13, 2022 · 5 comments
Closed
6 tasks done
Tracked by #833
Assignees
Labels
area/v3 relates to / is being considered for v3
Milestone

Comments

@Dokiys Dokiys closed this as not planned Won't fix, can't repro, duplicate, stale Oct 13, 2022
@meatballhat meatballhat reopened this Nov 27, 2022
@meatballhat meatballhat changed the title reduce our public api surface area, so there's less ways we can accidentally break people's code Minimize top-level API surface area for both usability clarity and maintenance reduction Nov 27, 2022
@meatballhat
Copy link
Member

(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
Copy link
Member

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

@meatballhat meatballhat added this to the Release 3.x milestone Nov 27, 2022
@meatballhat meatballhat self-assigned this Nov 27, 2022
@marwan-at-work
Copy link
Contributor

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

@meatballhat
Copy link
Member

@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

@dearchap dearchap added the area/v3 relates to / is being considered for v3 label Jun 12, 2023
@dearchap
Copy link
Contributor

dearchap commented Nov 2, 2024

Duplicate of #1774

@dearchap dearchap marked this as a duplicate of #1774 Nov 2, 2024
@dearchap dearchap closed this as not planned Won't fix, can't repro, duplicate, stale Nov 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/v3 relates to / is being considered for v3
Projects
None yet
Development

No branches or pull requests

4 participants