Command.main() now calls exit() with returnvalue of invoke()

[NiklasRosenstein]

see #270.

@unittake
I tend to agree, if you open a complete PR with doc updates and tests and changelog, I will merge it.

Testcase results are the same before and after the change (before, after).

[untitaker]

Please add a changelog entry and perhaps a versionchanged rst block (like versionadded in the same docstring)

[NiklasRosenstein]

@untitaker Unfortunately the tests for chained commands still fail, I couldn't find out where to solve it yet. Could you take a look?

[untitaker]

Ok. Here's the problem:

@click.command()
def foo():
   return 0

If you run foo.main(['foo'], standalone_mode=False), you will get a 0 as return value. However, if you have a command group with chain=True and the subcommands a, b, c, and you invoke them like this:

foo.main(['a', 'b', 'c'], standalone_mode=False)

You currently get [0, 0, 0] back, and your current code tries to run sys.exit([0,0,0]).

It's much less obvious what to do here. Should we use the first non-zero, non-none value as exit code? Or...?

[untitaker]

Also it appears you can interrupt the chain by calling sys.exit(0) in one subcommand. If I raise SystemExit(0) from a, then b and c will not run but the exit code will be 0. How should we behave if one returns 0 from a command? Should we interrupt the chain too?

[NiklasRosenstein]

Thanks for taking a look. Imho, interrupting the chain and returning the last command's exit code makes the most sense.

[untitaker]

So if we return 0 we should interrupt the chain?

[untitaker]

TBH at this point I think this is too much of compatibility breakage to add, for very little benefit.

[NiklasRosenstein]

So if we return 0 we should interrupt the chain?

No, on non-zero return, non-zero sys.exit() or any other exception. Just as if the command chain was invoked via the shell && operator, like cli foo && cli bar && cli baz. I don't think that behaviour would affect compatibility much. Alternatively, the chain parameter could support an || and && mode. Making || the default (and mapping chain=True to ||) would maintain full backwards compatibility except for the change of the return value to only return the result of the last command, which I deem a necessary and logical change.

[untitaker]

The thing is that sys.exit(0) breaks the chain already.

…

On April 18, 2017 9:25:03 PM GMT+02:00, Niklas Rosenstein ***@***.***> wrote: > So if we return 0 we should interrupt the chain? No, on non-zero return, non-zero sys.exit() or any other exception. Just as if the command chain was invoked via the shell `&&` operator, like `cli foo && cli bar && cli baz`. I don't think that behaviour would affect compatibility much. Alternatively, the `chain` parameter could support an `||` and `&&` mode. Making `||` the default (and mapping `chain=True` to `||`) would maintain full backwards compatibility except for the change of the return value to only return the result of the last command, which I deem a necessary and logical change. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #765 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[NiklasRosenstein]

You can catch it as SystemExit and check if the exit code is any of (0, None) and dismiss it in that case if a nother command is to be executed in the chain.

[untitaker]

Right, but at that point we would be breaking backwards compat because raising SystemExit already unconditionally interrupts the chain

…

On April 19, 2017 7:43:14 AM GMT+02:00, Niklas Rosenstein ***@***.***> wrote: You can catch it as `SystemExit` and check if the exit code is any of `(0, None)` and dismiss it in that case if a nother command is to be executed in the chain. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #765 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[untitaker]

I will close this because this behavior would be just too complex for the little value added. Sorry for noticing this problem so late.

[Kamekameha]

I might be missing your point or something but what's wrong with simply raising a ClickException instance? Like you can look literally 4 lines bellow your change and see that click already catches ClickException exceptions and exit the interpreter with the exit code associated with the exception (i.e. with the ClickException.exit_code attribute), so maybe you could subclass and customize said exception to your needs and you could technically exit your CLI with virtually any exit code.

Bringing this up 'cause nobody mentioned this here nor in #270 when it's already in my opinion a viable approach. Also functions' return values are already used for chained commands that relies on MultiCommand.resultcallback() so basically any modification with regards to commands' return values is already an API breakage for click (see, for example, http://click.pocoo.org/6/commands/#multi-command-pipelines).

[untitaker]

Your approach is entirely valid, but solves a different problem. This PR doesn't add new capabilities or enable a new usecase, it would've been a cosmetic change.

…

On April 21, 2017 5:58:35 AM GMT+02:00, Kamekameha ***@***.***> wrote: I might be missing your point or something but what's wrong with simply raising a `ClickException` instance? Like you can look literally 4 lines bellow your change and see that `click` already catches `ClickException` exceptions and exit the interpreter with the exit code associated with the exception (i.e. with the `ClickException.exit_code` attribute), so maybe you could subclass and customize said exception to your needs and you could technically exit your CLI with virtually any exit code. Bringing this up 'cause nobody mentioned this here nor in #270 when it's already in my opinion a viable approach. Also functions' return values are already used for chained commands that relies on `MultiCommand.resultcallback()` so basically any modification with regards to commands' return values is already an API breakage for `click` (see, for example, http://click.pocoo.org/6/commands/#multi-command-pipelines). -- You are receiving this because you modified the open/close state. Reply to this email directly or view it on GitHub: #765 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[Kamekameha]

Oh, no no, my suggestion isn't trying to solve any problem at all, but neither is this PR. As you've noticed, this breaks chained multi-commands, and exit codes by function return value don't make that much sense for those either. How would you discern a valid return value of, for example, 1 from an actual exit code?

Just to be clear about my previous comment, what I meant is that return values on commands have already a documented behavior and design, and signaling errors is what I believe exceptions are for. To me, this looks more like a behavioral change rather than cosmetics.

[untitaker]

I agree with you, this is why I closed this PR.

…

On Fri, Apr 21, 2017 at 07:09:38AM -0700, Kamekameha wrote: Oh, no no, my suggestion isn't trying to solve any problem at all, but neither is this PR. As you've noticed, this breaks chained multi-commands, and exit codes by function return value don't make that much sense for those either. How would you discern a valid return value of, for example, `1` from an actual exit code? Just to be clear about my previous comment, what I meant is that return values on commands have already a documented behavior and design, and signaling errors is what I believe exceptions are for. To me, this looks more like a behavioral change rather than cosmetics. -- You are receiving this because you modified the open/close state. Reply to this email directly or view it on GitHub: #765 (comment)