Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CliRunner: restrict mix_stderr influence to <output>; keep <stderr> and <stdout> stable #2522

Open
kdeldycke opened this issue May 30, 2023 · 1 comment · May be fixed by #2523
Open

CliRunner: restrict mix_stderr influence to <output>; keep <stderr> and <stdout> stable #2522

kdeldycke opened this issue May 30, 2023 · 1 comment · May be fixed by #2523
Milestone

Comments

@kdeldycke
Copy link
Contributor

kdeldycke commented May 30, 2023

Let's start with the current behaviour of CliRunner regarding its mix_stderr option.

Here is a simple CLI:

from click import command, echo
from click.testing import CliRunner


@command()
def hello():
    echo("1 - Hello world")
    echo("2 - Hello error", err=True)
    echo("3 - Good bye world")
    echo("4 - Good bye error", err=True)


if __name__ == "__main__":

    print("\n*** mix_stderr=True (default) ***\n")
    runner = CliRunner(mix_stderr=True)
    result = runner.invoke(hello)
    print(f"--- <stdout> ---\n{result.stdout}")
    print(f"--- <stderr> ---\n(raises error)\n")
    print(f"--- <output> ---\n{result.output}")

    print("\n*** mix_stderr=False ***\n")
    runner = CliRunner(mix_stderr=False)
    result = runner.invoke(hello)
    print(f"--- <stdout> ---\n{result.stdout}")
    print(f"--- <stderr> ---\n{result.stderr}")
    print(f"--- <output> ---\n{result.output}")

When called, the CLI above produces:

$ python ./cli_runner_stderr_test.py

*** mix_stderr=True (default) ***

--- <stdout> ---
1 - Hello world
2 - Hello error
3 - Good bye world
4 - Good bye error

--- <stderr> ---
(raises error)

--- <output> ---
1 - Hello world
2 - Hello error
3 - Good bye world
4 - Good bye error


*** mix_stderr=False ***

--- <stdout> ---
1 - Hello world
3 - Good bye world

--- <stderr> ---
2 - Hello error
4 - Good bye error

--- <output> ---
1 - Hello world
3 - Good bye world

What I'd like to propose is to instead have CliRunner produce this result:

$ python ./cli_runner_stderr_test.py

*** mix_stderr=True (default) ***

--- <stdout> ---
1 - Hello world
3 - Good bye world

--- <stderr> ---
2 - Hello error
4 - Good bye error

--- <output> ---
1 - Hello world
2 - Hello error
3 - Good bye world
4 - Good bye error


*** mix_stderr=False ***

--- <stdout> ---
1 - Hello world
3 - Good bye world

--- <stderr> ---
2 - Hello error
4 - Good bye error

--- <output> ---
1 - Hello world
3 - Good bye world

So what I propose is to streamline the semantics of result.stdout, result.stderr and result.output:

  • Let result.stdout always contain the pure output to <stdout>. Never mangle <stderr> in it.
  • Let result.stderr always contain the pure output to <stderr>. Never raise an error.
  • Use result.output as the content the user is expected to see:
    • let it be a proxy of <stdout> if mix_stderr=False
    • have it produce a mix of <stdout> and <stderr> if mix_stderr=True

The advantage of this is we could properly check the individual <stdout> and <stderr> streams, and check for the user output by inspecting <output>.

The raised exception in <stderr> has been introduced in 7.0 for backward compatibility, because the code prior to 7.0 wasn't returning the stderr/stderr split output (see #868). It's from 2017 so we can safely change that behaviour by now.

Note that I explicitely numbered each echo() statement to highlight the print order. If a refactor is attempted, I am anticipating some edge-cases around the preservation of order, so it's good to have simple code to test that.

@kdeldycke kdeldycke changed the title CliRunner: restrict mix_stderr influence to output only; keep <stderr> and <stdout> constant CliRunner: restrict mix_stderr influence to <output> only; keep <stderr> and <stdout> constant May 30, 2023
@kdeldycke kdeldycke changed the title CliRunner: restrict mix_stderr influence to <output> only; keep <stderr> and <stdout> constant CliRunner: restrict mix_stderr influence to <output>; keep <stderr> and <stdout> stable May 30, 2023
kdeldycke added a commit to kdeldycke/click that referenced this issue May 30, 2023
@kdeldycke kdeldycke linked a pull request May 30, 2023 that will close this issue
6 tasks
kdeldycke added a commit to kdeldycke/click that referenced this issue May 30, 2023
kdeldycke added a commit to kdeldycke/click that referenced this issue May 30, 2023
kdeldycke added a commit to kdeldycke/click that referenced this issue May 30, 2023
kdeldycke added a commit to kdeldycke/click that referenced this issue May 30, 2023
@kdeldycke
Copy link
Contributor Author

I just finished a PR at: #2523

It is ready to be reviewed and/or merged upstream.

kdeldycke added a commit to kdeldycke/click that referenced this issue Jun 29, 2023
@davidism davidism added this to the 8.2.0 milestone Jul 4, 2023
kdeldycke added a commit to kdeldycke/click that referenced this issue Jul 9, 2023
kdeldycke added a commit to kdeldycke/click that referenced this issue Jul 9, 2023
kdeldycke added a commit to kdeldycke/click that referenced this issue Aug 31, 2023
kdeldycke added a commit to kdeldycke/click that referenced this issue Aug 31, 2023
kdeldycke added a commit to kdeldycke/click that referenced this issue Aug 31, 2023
kdeldycke added a commit to kdeldycke/click that referenced this issue Sep 2, 2023
kdeldycke added a commit to kdeldycke/click that referenced this issue Sep 2, 2023
kdeldycke added a commit to kdeldycke/click that referenced this issue Feb 23, 2024
kdeldycke added a commit to kdeldycke/click that referenced this issue Feb 23, 2024
Closes pallets#2522

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants