Cli: maint: release 3.X

[lynncyrin]

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 changes want to consider for V3:

• moving altsrc into the main package
• taking a pass at reducing our public api surface, so there's less ways we can accidentally break people's code
• remove EnableBashCompletion in favor of CompletionShells
• consider a different release method (eg. not just merging to master) via https://github.com/urfave/cli/issues/952#issuecomment-561062742
• ~make flags "global" by default, to some degree~ this was added into v2!
• https://github.com/urfave/cli/issues/783#issuecomment-559643240
• Improve testability of *cli.Context (via https://github.com/urfave/cli/issues/833#issuecomment-574171962)
• removing unused fields such as Compiled

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

[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. 💚

[lynncyrin]

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

[lynncyrin]

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

[lynncyrin]

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

[lynncyrin]

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

[lynncyrin]

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?

[lynncyrin]

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!

[lynncyrin]

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

[lynncyrin]

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