Release 3.x meta/discussion

[coilysiren]

This is a meta-issue meant to track the work in other issues targeting the Release 3.x milestone.

benevolent takeover notes

Hiii I'm working on getting many maintenance tasks addressed including
taking over this issue from @coilysiren 🙇🏼 who is busy with other life adventures ❤️ ~ @meatballhat

I know 2.0 isn't out yet 🙂 (see #826) but I wanted to create a tracking issue for the 3.x series.

Here's some breaking/difficult changes we want to consider for V3:

• Cleanup redundant code in App/Command #1498
• Fix: Help name consistency among app/commands and subcommands #1489
• Replace code generation with generics where possible/maintainable (First cut at using generics for base flag types #1512 )
• merge altsrc with main package #1058
• Move man page compatibility to separate package #1055
• Add testify, and update tests to use it rather than expect #1695
• Improve testability of *cli.Context (via Release 3.x meta/discussion #833 (comment)) (conflicts with Consolidate all Context code into Command #1587)
• remove EnableBashCompletion in favor of CompletionShells
• consider a different release method (eg. not just merging to master) via Support users who are using GOPATH mode #952 (comment) (this was changed in v2)
• make flags "global" by default, to some degree (this was added into v2)
• Compiled metadata is not used #753
• Add optional check for duplicated string and int type flags. #1071
• Cleanup error handling. Make multiError implement ExitCoder
• Add integration with golangci-lint #1663
• Strictly limit default dependencies to stdlib #1890 ¹
• Ensure primary library usage is via struct literals #1774
• Ensure all exported structs are usable as "zero value" #1775
• Define base error type and use stdlib error wrapping #1662
• Minimize top-level API surface area for both usability clarity and maintenance reduction #1531
• Use mkdocs mike for multi-version docs #1885
• Build out all of the v3 documentation #1891

Please suggest more! I'll add them to this OP

Update ~January 2023: Progress! And yet, yeah, wish lists balanced with releasing usable software is hard to do. If you're the kind of human who finds themselves reading this, there's a v3.0.0-alpha2 tag now, which should play nicely with go get github.com/urfave/cli/v3@latest. Feedback wanted! ❤️ ~ @meatballhat

Footnotes

1. The documentation-related code that depends on github.com/russross/blackfriday/v2 and github.com/cpuguy83/go-md2man/v2 has been shown to add significant bulk to compiled outputs ↩

[saschagrunert]

Regarding the API we could split it up into an internal and external one. Everything which should not be imported externally would go into the internal folder.

[saschagrunert]

I would love to see fish shell completion support. 💚

[coilysiren]

Regarding the API we could split it up into an internal and external one. Everything which should not be imported externally would go into the internal folder.

I'm a big fan of tossing tons of code into internal, its a really cool pattern

[coilysiren]

I would love to see fish shell completion support.

For anyone else reading, here's a ticket for that => #351

[coilysiren]

Oh! I see now that we call the flag EnableBashCompletion instead of CompletionShells. I'll add this to the OP 👍

[marwan-at-work]

Since v2 was never officially released, it might be okay to make minimal breaking changes to the v2 branch before merging and releasing it?

I'm happy either way, but if that's more convenient then it should be okay since people know that v2 has not been released yet and so it's okay to make some breaking changes before its officially stable

[coilysiren]

^ I'm cleaning up the issues about altsrc ^_^

[marwan-at-work]

Pass (context.Context, *Command) to action funcs instead of (*Context)

Since this is v3 and we can make breaking changes from v2, why not just let *Context implement context.Context. All you need to do is change the Value method's name to be something like GetValue

[dearchap]

Interesting. Would you like to submit a PR ?

[marwan-at-work]

@dearchap here you go #1597

[meatballhat]

whoops not quite closed, there!

[meatballhat]

@marwan-at-work thank you for #1597 ! What about un-embedding context.Context and having it as an explicit named member instead? IIUC the stdlib context.Context is explicitly not meant to be extended, so I feel weird about committing further to what I consider a wart of the v2 API.

[meatballhat]

@marwan-at-work fwiw I'd vote for a field named Ctx so that it's more recognizably a completely different type than *cli.Context

[marwan-at-work]

context.Context is explicitly not meant to be extended

@meatballhat any documentation or links to where that explicit convention maybe written?

What is the downside here anyway?

IMHO, the fact that context.Context was embedded and only half-implemented the interface, was odd. But that was because we couldn't introduce a breaking change to the Value method. But with v3, you can go all the way and have *cli.Context implement the whole interface, or you can go all the way in the opposite direction and not embed context.Context at all, but give access through a field like you suggested here:

fwiw I'd vote for a field named Ctx so that it's more recognizably a completely different type than *cli.Context

That works for me too, but would you want *cli.Context to implement the interface, or to not implement the interface but give access to the underlying context through the field? If the latter, you don't need to rename it to Ctx, you can just un-embed it by calling it Context and this way it won't be a breaking change, because *cli.Context in v2 never implemented context.Context, and urfave/cli/v2 users don't need to make further changes to their code.

[meatballhat]

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

I'd love to keep talking about this, and I also want to be sensitive to keeping discussions here very high-level, so I'm copying these comments over to #1531 for further discussion 🙇🏼

[marwan-at-work]

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

Update: Progress!¹ And yet, yeah, wish lists balanced with releasing usable software is hard to do. If you're the kind of human who finds themselves reading this, there's a v3.0.0-alpha2 tag now, which should play nicely with go get github.com/urfave/cli/v3@latest. Feedback wanted! ❤️

Footnotes

1. this comment also available in issue description ↩

[xoxys]

Looks like GenericFlags are removed in v3, is there a replacement for them?

[dearchap]

@xoxys imo GenericFlag was really not that useful since it depended on the user providing a Generic instance to the GenericFlag anyway. Same thing can be achieved in v3 by providing a Value and ValueCreator instance to FlagBase in-lieu of implemented a new flag instance. Let me know if this isnt sufficient for you needs. We can look into simplying it.

[xoxys]

Hi @dearchap sorry for the delay. Maybe my question was not accurate enough. What I'm doing today with v2 is using a custom type https://github.com/thegeeklab/wp-plugin-go/blob/main/types/stringslice.go that is then used in a generic flag https://github.com/thegeeklab/wp-docker-buildx/blob/main/cmd/wp-docker-buildx/config.go#L331-L336

I'm not even sure if that is the proper way to do it with v2 🙈 but how could this be done in v3? At the end, I'm looking for a way to implement a custom flag that can do some custom data manipulation.

[dearchap]

@xoxys You would use the FlagBase directly instead of SliceBase, so you would define FlagBase[string[], cli.NoConfig, ]

The value creator will basically return the string slice impl value that you already have. Take a look at any of the current flag_.go to see how a valueCreator works. Let me know if you need any help reg that

[serprex]

@meatballhat our usecase was simple at PeerDB-io/peerdb#944 & converting was straightforward. Bit of a hiccup figuring out App/Command merge since v2/v3 migration guide doesn't mention App being removed

Our usecase might be simple enough that we should just use flag, but this module seems lightweight enough. Looking forward to final v3 release

[bartekpacia]

Hi! Thank you very much for maintaining this awesome module. When do you expect v3 beta release? Is there anything a random contributor could help with? I have some free time :)

[dearchap]

@bartekpacia The only thing preventing us from a beta release is a new arg parser from either @meatballhat or @jtagcat .

[jtagcat]

harg exists as a complete parser, I moved it under hcli (WIP, my 'urfave/cli v3') to try integrating it with a CLI wrapper. The place I got stuck with was the logic of nested subcommands.

It has been on the back burner for a while. I've been annoyed for there not being no good cli lib for Go a few times, but instead slapped things together without a library at all.

While it still is on my TODO; I haven't planned for it. Might do it one day, might not.

I think it got noted before, that harg isn't a drop-in to urfave/cli.

[bartekpacia]

Alright, thanks!

Are you planning on making some breaking changes? I know such questions might be hard to answer right now since well, it's alpha, but I'd appreciate an answer anyway :-)

Meanwhile I think it would make sense to also release documentation for v3 on the docs website (and of course name it like v3 (alpha). Possibly relevant issue: #1801

[jtagcat]

At least my version, yes, breaking.

I don't agree with releasing docs before the exact design/spec is down. My effort is probably separate from urfave/cli (see top post). If you want it, you'll likely want to build it yourself, or buy the work. This issue is 5y old.

[meatballhat]

@bartekpacia The only thing preventing us from a beta release is a new arg parser from either @meatballhat or @jtagcat .

I'd like to include #1885 and then creating a whole new set of docs for the v3 feature set in the list of stuff to do before releasing a v3 beta 💖