dwreeves
commented
6 months ago I'll admit I am a little confused about something with your original report and this PR, I'm hoping there is a way we can resolve it.
I have a file called foo.py with the following contents:
# import rich_click as click
import click
@click.group("my-command")
@click.argument("foo")
@click.option("--bar", "-b", help="Lorem ipsum", show_default="someval")
def cli(foo, bar):
"""Help text for CLI"""
@cli.command("subcommand")
def subcommand(foo, bar):
"""Help text for subcommand"""
if __name__ == "__main__":
cli()And I use this script to execute foo.py and examine the outputs:
from subprocess import Popen, PIPE
process = Popen(["python", "foo.py", "--help"], stdout=PIPE, stderr=PIPE)
stdout, stderr = process.communicate()
print(f"{len(stdout)=}", f"{len(stderr)=}")When I run it with import click, I get the following: len(stdout)=225 len(stderr)=0
When I run with import rich_click as click, I get the following: len(stdout)=3447 len(stderr)=0
When I run with import rich_click as click, and also use your code change, I get the following: len(stdout)=1 len(stderr)=3446
Which is to say, the default behavior of writing to stdout seems correct and in line with what Click does, but this PR seems to change it so the default behavior is that it writes to stderr.
What I thought you had in mind when you said we aren't printing to stderr was more something like this. The following code adds a --fake-flag to trigger an error:
from subprocess import Popen, PIPE
process = Popen(["python", "foo.py", "--fake-flag"], stdout=PIPE, stderr=PIPE)
stdout, stderr = process.communicate()
print(f"{len(stdout)=}", f"{len(stderr)=}")When I run it with import click, I get the following: len(stdout)=0 len(stderr)=117
When I run with import rich_click as click, I get the following: len(stdout)=2074 len(stderr)=0
^ This to me is erroneous behavior on rich-click's part. By default, rich-click should write to stdout, it's just in error-handling situations that it should be writing to stderr, but it isn't.
Do you agree with everything I've stated? Let me know if anything here looks wrong.
In 1.8.0, which I can potentially release an alpha version of soon (mostly, I'm just waiting to finish the docs, aaah), you'll notice the __init__ for RichHelpFormatter looks different than in 1.7.x:
def __init__(
self,
indent_increment: int = 2,
width: Optional[int] = None,
max_width: Optional[int] = None,
*args: Any,
console: Optional["Console"] = None,
config: Optional[RichHelpConfiguration] = None,
file: Optional[IO[str]] = None,
**kwargs: Any,
) -> None:TLDR, 1.7.0 was a massive shake-up in rich-click's internals, but some of the internals were not fully fleshed out, and so a big part of what 1.8.0 is doing is making those internals better.
There is potential, somewhere in the code, to initialize RichHelpFormatter() with file=sys.stderr in error handling contexts, and this would resolve the issue.
A related issue (which is not a prerequisite for contributing a fix; this can be worked on separately as it's a bigger matter) is that the test suite right now does not distinguish between stderr or stdout, but it should. Hence why your tests are passing. I do believe that the tests should distinguish between those two things.
I will say, in the dev branch for 1.8.0a, we are currently not writing to stderr for error messages. So the erroneous behavior you've flagged is still existent in 1.8.0.
This is something we could potentially address in 1.8.0, and it may be difficult to address in 1.7.x. And in that case, I'm really sorry I encouraged you to push a change to 1.7.x 😦.
TLDR of everything:
- Based on what I can tell,
stderrshould be written to only for error messages - rich-click does not currently do that, hence that is a bug.
- It is possible but may be difficult to coherently implement this change in
1.7.x, however1.8.0's internals have been reworked a bit and it should be a little easier there. - Again, really sorry I encouraged you to push changes to
1.7.x; I was typing that from work and didn't have time to fully investigate everything relating to the matter. But yeah, it may make more sense to do it there given the internals are better suited for something like this.
Julian
Matches click's behavior.
Closes: #164