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 https://github.com/urfave/cli/issues/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:

• [x] #1498
• [x] #1489
• [x] Replace code generation with generics where possible/maintainable (#1512 )
• [x] ~#1058~
• [x] #1055
• [x] #1695
• [x] ~Improve testability of *cli.Context (via https://github.com/urfave/cli/issues/833#issuecomment-574171962)~ (conflicts with #1587)
• [x] remove EnableBashCompletion in favor of CompletionShells
• [x] ~consider a different release method (eg. not just merging to master) via https://github.com/urfave/cli/issues/952#issuecomment-561062742~ (this was changed in v2)
• [x] ~make flags "global" by default, to some degree~ (this was added into v2)
• [x] #753
• [x] #1071
• [x] ~Cleanup error handling. Make multiError implement ExitCoder~
• [x] #1663
• [x] #1890 [^1]
• [x] #1775
• [ ] #1774
• [ ] #1662
• [x] #1531
• [ ] #1885
• [ ] #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

[^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 => https://github.com/urfave/cli/issues/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 ^_^

[coilysiren]

I'm gonna drop off of guidance for this one - I've got a few other things I can focus on

[torkelrogstad]

Commenting here, as it seems like you guys want everything related to altsrc in this issue. Thanks for all the work you've put in on a great package!

I've got two questions about usage of altsrc to read flags from a config file:

1) It seems like altsrc.InitInputSourceWithContext returns an error if there are some flags in the config file which aren't defined, even though said flags aren't marked as Required when defining them. Is this the intended behavior, or am I doing something wrong here?

2) If marking a flags as Required, I get an error when executing my command when defining the flag in my config file, but not at the CLI invocation point. This seems like a bug to me, or am I confused about something here?

[coilysiren]

Commenting here, as it seems like you guys want everything related to altsrc in this issue.

Yes please!

To your questions - I'm not sure yet what the answers are, but I will totally investigate o7!

[coilysiren]

Something I want to consider for v3: making all flags "generic", so you only have a "Flag" type and we use type assertions internally to turn it into StringFlag

[coilysiren]

It's looking like I'm not gonna have time to do a bunch of work on the V3 PR anytime in the near / mid future, so I'd like to hand it off to anyone else => https://github.com/urfave/cli/pull/936

Anyone is - who is so inclined - is also free to help guide this V3 tracking issue ✨

[rliebz]

Improve testability of *cli.Context. A couple of potential options:

• Create a cli.NewTestContext or similar that could give us the ability to configure private fields like flagSet that are currently impossible to set externally.
• Make cli.Context an interface, so cli.ActionFunc and friends could accept any type that implements those methods, rather than only a *cli.Context.

See discussion: https://github.com/urfave/cli/pull/1039#issuecomment-574089223

[asahasrabuddhe]

Improve testability of *cli.Context. A couple of potential options:

• Create a cli.NewTestContext or similar that could give us the ability to configure private fields like flagSet that are currently impossible to set externally.
• Make cli.Context an interface, so cli.ActionFunc and friends could accept any type that implements those methods, rather than only a *cli.Context.

See discussion: #1039 (comment)

This is an amazing idea

[rliebz]

taking a pass at reducing our public api surface, so there's less ways we can accidentally break people's code

Closed a while back due to staleness, but removing unused fields such as Compiled are a good candidate for reducing the public API surface: https://github.com/urfave/cli/issues/753

[asahasrabuddhe]

Interesting Feature: Make the library modular.

As mentioned in issue #1055 our library has grown quite large considering a plethora of features we are offering. While this is good, not all users are using every feature. This means that people are downloading a bunch of dependencies and not ending up using them.

Having a modular library would allow us make the core smaller and have the other features as addons or plugins.

[asahasrabuddhe]

When we were planning to release v2, there was some talk about how to go forward with the release. There were several chains of thought around the matter. I think we are at a point where we would soon release v3. I want everyone's help in deciding how are we going to go forward with the release.

Do we cut out a separate v2 branch like the v1 and continue bug fixes and other development etc for v2 over there and master becomes v3?

Does anyone have any other idea?

[asahasrabuddhe]

I also think that we should start with a development branch for v3 where we can merge the changes and keep it up-to-date with master until we decide to make the final release. I will check out this branch from master and point my PRs to that branch

[chickenandpork]

FWIW, Compiled can be useful when confirming a debug/canary version of software: typically if the version isn't YET bumped, the only macroscopic difference is compile date.

I see more use though of the text-based ISO8601/RFC3339 of the commit date in our typical version output so that repeated builds of the same commit reproduce the same date.

var (
    BUILD_DATE string
)
...
       if buildTime, err := time.Parse("2006-01-02T15:04:05-07:00", BUILD_DATE); err == nil {
               app.Compiled = buildTime
       }

Mind you, requires a build-time flag (based on git show --no-patch --no-notes --pretty='%cI' HEAD) to populate that, which is why I silently consume the error on parse to not set the date.

Might not appear useful, but next time you're wondering whether CI/CD pushed the right thing to canary, you might remember this comment.

[rliebz]

Drop explicit support for the MultiError interface in HandleExitCoder, and instead make the private multiError type implement ExitCoder.

Right now MultiError as an interface seems to be supported specifically for for the internal multiError type, but requires logic in the default handler, HandleExitCoder, which doesn't really seem ideal. Instead, if the private type happened to implement ExitCoder itself, the generic default handler can be simplified to only care about ExitCoders.

The main downside to this approach is that a MultiError couldn't easily conditionally implement an interface. Imagine a method like this:

func (m multiErr) ExitCode() int {
  var exitCoder ExitCoder
  for _, err := m.Errors() {
    if errors.As(err, &exitCoder) {
      return exitCoder.ExitCode()
    }
  }

  // No error in the multi-error was an exit coder, but the multi-error still is!
  // We have to return something here...
  return -1
}

To support that kind of case, we'd probably want to change the behavior of HandleExitCoder to specifically check for exit codes >=0 (or >0?), so a multi-error or other type of error could conditionally opt in-or-out of being an ExitCoder.

One other caveat is that because of the current implementation, any error implementing MultiError is actually treated as an ExitCoder and causes the application to exit with a status of 1, even if there is no ExitCoder in the MultiError. We probably don't want that to be the case.

Changing that behavior is technically a breaking change, so we wait until v3.

Related: #1089

[dearchap]

Is there any appetite for re-organizing code ? I have begun some basic cleanup is #1264 but my main goal is to cleanup and app.go and command.go files. There is a lot of duplicate code in these two files which makes it hard to maintain. Any new feature needs to be added in two places. Its a nightmare for maintainers. What I would suggest is to have all the processing logic in command.go. App would create an internal rootCmd(of *Command) and run with a default parent context. So all the logic is within Command and there is no need to switch from app to command and then back to app. All this subcommand logic in App goes away. Everything becomes neatly compacted into one function in Command. All the helpTemplates collapse into one and will be rendered with a Command object. Main app commands/flags would be rendered with rootCmd and command specific stuff with the corresponding Command object. It becomes all consistent and we wouldnt even have to change the user level API. All this happens behind the scenes without user having to change any code. I have a partial implementation of this which I'm testing out but dont want to proceed if it ends up getting rejected.

[rliebz]

Thank you for your interest and contributions, I really appreciate the effort you've put in so far.

Is there any appetite for re-organizing code ?

Absolutely. If there are ways to improve the code base that aren't breaking changes, we can slot those into v2, even if they're major refactors. I agree that there are lots of things we have that are less than ideal, and improvements are always welcome.

I have a partial implementation of this which I'm testing out but dont want to proceed if it ends up getting rejected.

Unfortunately I think the time that's actively been put into maintenance is less than I'd like it to be, which I say as a maintainer that wishes I spent more time on responding to issues/PRs. There's a very real possibility that if you put something wonderful together, it gets overlooked rather than rejected.

The best candid advice I can give is—if you aren't prepared to wait for a response, unfortunately now might not be the best time to contribute. I'll do my best to pitch in where I can, but for v2 changes you'll need at least two maintainers to approve. If you do want to contribute, a series of smaller changes are a lot easier to get merged in than trying to push a lot of things together in the same PR, we will definitely ask for unit tests for anything that's new or changing that doesn't already have tests, and absolutely feel free to ping multiple times if you don't get a response.

I realize that's probably not the best answer to hear, but it's the one I can give right now. Thank you again for your time here.

[dearchap]

@rliebz These are my thoughts exactly. I've been going through the PRs and issues and noticing the long lead time in responses/approvals. I dont mind waiting a month or so for a comment and 2-3 months for an approval. That absolutely fine with me. One of my PRs in kustomize has been "stuck" since August of last year. So I totally understand where you're coming from. Maintainers have so many other priorities that they cannot afford to spend time daily on one project.

If you look at my PRs for urfave/cli I've tried to break it down into very very small pieces, with unit tests included, so that it gives the reviewers confidence that nothing has broken and only some existing bug has been fixed. Without tests there is no way a maintainer can figure out just from the code(for non-trivial changes) whether the bug is fixed or not. I'm a big fan of unit tests. In fact I write the unit tests first to make sure the problem exists and then make changes to the code and then rerun all tests to make sure nothing is broken.

My refactoring of app/command is a very slow process. Sometimes I make a change and it works and sit on it for a while. After a couple of days a different idea pops up and I try to incorporate that. So I'm in no hurry to finish it asap even though that would be great :) My goal is to make sure that there are zero user facing changes for this. Its a big task but I think its worth it in the long run to make the code more modular and more maintainable.

Thank you for your comments and time. Appreciate it.

[heyvito]

Hey @lynncyrin (or maybe @rliebz?)! Could you kindly consider adding something so we can choose between how args and flags are parsed? There's a discussion over at #1113.

Not sure this is the right place to ask for that. I do apologise in case it isn't.

[rliebz]

The status of a 3.X release is pretty unlikely right now, since the maintenance overhead is probably higher than we have folks who can support it (although we haven't dropped support for v1 yet!).

That said we are still accepting backward-compatible contributions for v2, so definitely feel free to open or comment on individual issues. This issue in particularis probably best suited for discussing potential changes we might make as breaking changes if the situation changes in the future.

[meatballhat]

For anyone happening across this end of the discussion, I'm working on building up my understanding of this whole situation and I'm hoping to have an updated roadmap for 3.X in the OP in the next few days 🤞🏼

[asahasrabuddhe]

@meatballhat im also open to have a zoom or google meets meeting for us to get together and plan the future of this library.

[coilysiren]

😆 lol yes busy with other life adventures

[erikwilson]

FWIW this is an abandoned refactor: https://github.com/rancher/spur/

[meatballhat]

@meatballhat im also open to have a zoom or google meets meeting for us to get together and plan the future of this library.

Hello, @asahasrabuddhe! Sorry for not seeing this reply earlier 😩 If you're able, can we coordinate meeting via the team discussions? https://github.com/orgs/urfave/teams/cli

[dearchap]

- [ ] New flag parser, written from ground up to overcome golang flag limitations
- [ ] Remove public fields in App/Command structs and replace with an Options interface for configuration
- [ ] Single Flag interface which encapsulates all the functions in the other *Flag interfaces, Required(), Category() etc etc
- [ ] Target go 1.18+ as a minimum
- [ ] Remove generated flag code and replace with generics,
- [ ] Any standard type flags with also have *Slice types by utilizing generics.
- [ ] Minimal number of files, not including test files, app.go, command.go, context.go, help.go and types.go
- [ ] Collapse redundant execute code in app.go/command.go . Consider using an App.RootCommand to handle App.run

**NOTE: Ignore the list in this comment Please use the requirements at the top of this issue.** 

[jtagcat]

Single Flag interface which encapsulates all the functions in the other *Flag interfaces, Required(), Category() etc etc

Did i miss the boat and am bikeshedding?

With encapsulation, you have to be explicit every time. (The current) Required as a bool (struct embedding) is just fine. Defaults to false. With flag reuse, you can also copy flag definitions, and modify only that.

[dearchap]

@jtagcat All of these interfaces arose out of the original design as a way of extending the base interface and then we have hacks in the code depending on this interface and that. The current way of thinking is to encapsulates(as far as is currently known) all flag attributes into a single interface. We hope to use Generics to simplify new flags.

[dearchap]

@jtagcat I've been thinking about this a bit more and I think you have a point. Yes its best to keep the Flag interface to minimum and have extra functionality via extended interfaces. I can revert my changes for this.

[dearchap]

PR #1517

[dearchap]

I think have the Required interface for Flag creates more problems than it solves. I am considering removing it from v3. Any thoughts ?

[jtagcat]

I think have the Required interface for Flag creates more problems than it solves. I am considering removing it from v3. Any thoughts ?

Could you elaborate? If you mean removing the option to require cli end-user in explicitly setting an option, then I'm against it. I use it in most of my apps.

[dearchap]

@jtagcat Thanks for the quick feedback. Lets leave it as is then,

[meatballhat]

@jtagcat feedback also very much appreciated over at #1559 💖

[dearchap]

How about removing altsrc ? :)

[meatballhat]

How about removing altsrc ? :)

Interesting! What do you have in mind? Dropping it without replacement? Moving to a separate project? Other?

[dearchap]

Move it to a separate project. I dont mind dropping it as well. Its too much work for us to maintain this. The base urfave/cli should work well and allow flexible extensions to do altsrc stuff. In terms of maintenance altsrc is a can of worms, people want all sorts of features, subtrees, short paths etc etc. cli apps needs to focus on basic cli features and not nice file formats, json, yaml otherwise. Other than envs and cmdline args there is no reason a cli framework should support anything else.

[meatballhat]

@dearchap Understood! I'm happy to create github.com/urfave/cli-altsrc. Sound OK?

[quillaja]

I'm happy to see that there is a DefaultCommand option for the App. I would really like to see DefaultCommand on Command too.

[jtagcat]

cc @dearchap @meatballhat

Hm, so... https://github.com/jtagcat/harg is now functional (and fully GNU-compatible!). I can add the remaining tests within (or at the end of) next week.

I didn't really intend to, but this is a bit working against / competing with @dearchap's work, are you fine with it?

About harg:

1. It takes in map[string]*Definition: string being the key/argument/option name, Definition contains Type (enum)`.
   • Aliases can be added by pointing to the same Definition. There's also a convenience function for this.
   • Definition also contains AlsoBool, but for now, urfave/cli can just ignore it.
   • It looks like this:
2. args, chokeReturn, err := defs.Parse(os.Args[1:], chokes)
   • chokes: This may be ignored for now (use _ and nil). It allows specifying valid subcommands. Through multiple defs.Parse(chokeReturn, ...) calls, it enables local options/arguments.
   • args: non-flag options/args
3. defs[flagName] can be used to retrieve values:
   • .Touched(): if any flag was specified. This is to differentiate no flags and default values.
   • .SlString(), .SlInt64(), ...: Give you all values specified.
   • .String(), .Int64(), ...: Give you the last value specified.

That's all.

[meatballhat]

@jtagcat Are you planning to propose harg for #1583 and if so are you planning to open a pr?

[jtagcat]

@jtagcat Are you planning to propose harg for #1583 and if so are you planning to open a pr?

@meatballhat yes. I'm also in for working towards v3 (args), as it's annoying me and others daily. Though as it is from my own spare time, I, as before, can't promise anything done in a certain timeframe.

Moving to #1583 with anything flags-harg.

[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 https://github.com/urfave/cli/pull/1597