Exit codes aren't being propogated.

[Qwerty-133]

To reproduce run the following snippet of code:

# foo.py
import rich_click as click

@click.command()
@click.pass_context
def foo(ctx: click.Context) -> None:
    print('exiting')
    ctx.exit(2)
    print('exited')

foo()

Invoking as python foo.py || echo $? prints just "exiting", which means the command is not failing.
After replacing the import with import click, python foo.py || echo $? correctly prints "exiting" followed by "2".

click                     8.1.3
rich                      13.4.2
rich-click                1.6.1

[raabf]

I can confirm that.

The ctx.exit() method does an

raise click.exceptions.Exit(exit_code=2)

and the problem might come from the fact that Exit is not a subclass of ClickException: https://github.com/pallets/click/blob/ca5e1c3d75e95cbc70fa6ed51ef263592e9ac0d0/src/click/exceptions.py#L279-L289

And it looks like just ClickException and Abort is handled:

rich-click/src/rich_click/rich_command.py

Lines 59 to 74 in 12f52d2

	try:
	rv = super().main(*args, standalone_mode=False, **kwargs)
	if not standalone_mode:
	return rv
	except click.ClickException as e:
	rich_format_error(e, formatter)
	if not standalone_mode:
	raise
	sys.stderr.write(formatter.getvalue())
	sys.exit(e.exit_code)
	except click.exceptions.Abort:
	rich_abort_error(formatter)
	if not standalone_mode:
	raise
	sys.stderr.write(formatter.getvalue())
	sys.exit(1)

[dwreeves]

In order to achieve full parity in behavior with Click, it looks like we will need to re-implement all of click.BaseCommand.main().

The reason why is because a return value from main() when standalone_mode=False can be either an exit code or a return from the callback. The comments in the click code explain the situation:

https://github.com/pallets/click/blob/6986b08a7e4107e23f81ec7101d862e7116bc574/src/click/core.py#L1108-L1115

So if we want rich-click to be a true drop-in replacement for Click, it seems the only real solution here is to just re-implement the entire method.

This is not ideal due to forward compatibility concerns. But I think it may be the best I can do. We can possibly raise an issue in the future and see if the Pallets team would be amenable to some sort of abstraction that makes this easier to do.

Anyway, I'm working on this in a test-driven way. We have 4 behaviors we want to make sure work across both groups and commands, so 8 tests total. Here's the test file so far:

# tests/test_exit_code.py
import sys

from click.testing import CliRunner

from rich_click import command, group, RichContext, pass_context


# Don't use the 'invoke' fixture because we want control over the standalone_mode kwarg.

def test_command_exit_code_with_context():

    for expected_exit_code in range(10):

        @command("cli")
        @pass_context
        def cli(ctx: RichContext):
            ctx.exit(expected_exit_code)

        runner = CliRunner()
        res = runner.invoke(cli, [])
        assert res.exit_code == expected_exit_code


def test_group_exit_code_with_context():

    for expected_exit_code in range(10):

        @group("cli")
        @pass_context
        def cli(ctx: RichContext):
            ctx.exit(expected_exit_code)

        @cli.command("subcommand")
        @pass_context
        def subcommand(ctx: RichContext):
            ctx.exit(999)

        runner = CliRunner()
        res = runner.invoke(cli, ["subcommand"])
        assert res.exit_code == expected_exit_code


def test_command_exit_code_with_sys_exit():

    for expected_exit_code in range(10):

        @command("cli")
        def cli():
            sys.exit(expected_exit_code)

        runner = CliRunner()
        res = runner.invoke(cli, [])
        assert res.exit_code == expected_exit_code


def test_group_exit_code_with_sys_exit():

    for expected_exit_code in range(10):

        @group("cli")
        def cli():
            sys.exit(expected_exit_code)

        @cli.command("subcommand")
        def subcommand():
            sys.exit(999)

        runner = CliRunner()
        res = runner.invoke(cli, ["subcommand"])
        assert res.exit_code == expected_exit_code


def test_command_return_value_does_not_raise_exit_code():

    @command("cli")
    def cli():
        return 5

    runner = CliRunner()
    res = runner.invoke(cli, [])
    assert res.exit_code == 0


def test_group_return_value_does_not_raise_exit_code():

    @group("cli")
    def cli():
        return 5

    @cli.command("subcommand")
    def subcommand():
        return 10

    runner = CliRunner()
    res = runner.invoke(cli, [])
    assert res.exit_code == 0


def test_command_return_value_is_exit_code_when_not_standalone():

    for expected_exit_code in range(10):

        @command("cli")
        @pass_context
        def cli(ctx: RichContext):
            ctx.exit(expected_exit_code)

        runner = CliRunner()
        res = runner.invoke(cli, [], standalone_mode=False)
        assert res.return_value == expected_exit_code


def test_group_return_value_is_exit_code_when_not_standalone():

    for expected_exit_code in range(10):

        @group("cli")
        @pass_context
        def cli(ctx: RichContext):
            ctx.exit(expected_exit_code)

        @cli.command("subcommand")
        @pass_context
        def subcommand(ctx: RichContext):
            # I should not run
            ctx.exit(0)

        runner = CliRunner()
        res = runner.invoke(cli, ["subcommand"], standalone_mode=False)
        assert res.return_value == expected_exit_code

I should have something soon!

[dwreeves]

Done. This will be fixed in 1.7.0.

[raabf]

So if we want rich-click to be a true drop-in replacement for Click, it seems the only real solution here is to just re-implement the entire method.

Oh yea this is a maintenence hell 😞

This is not ideal due to forward compatibility concerns. But I think it may be the best I can do.

Thanks for implementing this anyway 😄 ! It is still bad in the meantime if exit code is wrong

Anyway, I'm working on this in a test-driven way. We have 4 behaviors we want to make sure work across both groups and commands, so 8 tests total. Here's the test file so far:

Great, Well I also detected this issue, because my unit-tests fail when rich-click is installed, but succeed without it 😅