Subcommand tree view#270
Conversation
|
Thank you so much for this speedy PR and I'm super excited to get it in for 1.9.0. It looks like the main test that is failing here is a test that asserts The reason we assert this is because importing rich slows down code execution by a few milliseconds, and increases the memory footprint. And we pride ourselves on doing the best we can to make sure we get in the way of code execution as little as possible 😄 See this:
(note: rich-click 1.9.0 is even better! because we found another optimization...) The way we get around this is through lazy imports, and you'll see examples of this throughout the code. A few examples: Here is an example of a lazy import direct to rich: rich-click/src/rich_click/rich_help_formatter.py Lines 40 to 52 in b77ce9e This example here is a lazy import to the rich-click/src/rich_click/rich_command.py Lines 265 to 268 in b77ce9e |
dwreeves
left a comment
There was a problem hiding this comment.
How much do you want to work on this vs how much do you want to pass it off to me?
If you get tests passing, I'm fine merging as is and taking it from here! But if we do that, I will be making a few changes to your code, mostly based on the things I outlined below.
If you'd like to see this through to the finish line, then you can go through the review comments I left.
I appreciate all your hard work on this ❤️ You did a great job and it's mostly in a pretty good spot. I'm really excited to see this in rich-click 1.9.0. 😄
| finally: | ||
| sys.exit(1) | ||
|
|
||
| def make_context( |
There was a problem hiding this comment.
It should not be necessary to override the make_context.
The issue you are running into I believe is because you are not modifying the class RichContext to take in the tree_option_names kwarg.
|
|
||
| Skipped if :attr:`add_help_option` is ``False`` (reusing the flag for simplicity). | ||
| """ | ||
| tree_option_names = self.context_settings.get("tree_option_names") |
There was a problem hiding this comment.
You should be reading this from a self.get_tree_option_names(ctx), not self.context_settings, where get_tree_option_names is mirrored after the inherited get_help_option_names method.
Reading from self.context_settings actually beats a lot of the point of supporting ctx.tree_option_names. One of the points of supporting this is to propagate the --tree option down to subcommands (via generation of child contexts).
| @@ -0,0 +1,227 @@ | |||
| import re | |||
There was a problem hiding this comment.
Thank you for writing tests! ❤️ I really appreciate it.
Can you please use inline_snapshot? https://github.com/15r10nk/inline-snapshot I promise you'll like it 😉 it's pretty neat.
Examples of this tool are in the tests/help/ directory, e.g.
def test_simple_help(cli_runner: CliRunner, cli: rich_click.RichCommand) -> None:
result = cli_runner.invoke(cli, "--help")
assert result.exit_code == 0
assert result.stdout == snapshot(
"""\
\n\
Usage: cli [OPTIONS] COMMAND [ARGS]... \n\
\n\
My amazing tool does all the things. \n\
This is a minimal example based on documentation from the 'click' package. \n\
You can try using --help at the top level and also for specific subcommands. \n\
\n\
╭─ Options ────────────────────────────────────────────────────────────────────────────────────────╮
│ --debug/--no-debug -d/-n Enable debug mode. Newlines are removed by default. │
│ Double newlines are preserved. │
│ --help Show this message and exit. │
╰──────────────────────────────────────────────────────────────────────────────────────────────────╯
╭─ Commands ───────────────────────────────────────────────────────────────────────────────────────╮
│ download Optionally use short-help for the group help text │
│ sync Synchronise all your files between two places. Example command that doesn't do much │
│ except print to the terminal. │
╰──────────────────────────────────────────────────────────────────────────────────────────────────╯
"""
)
assert result.stderr == snapshot("")| @@ -0,0 +1,40 @@ | |||
| from __future__ import annotations | |||
There was a problem hiding this comment.
I'm going to be honest, I'm not sure I am a fan of a TreeRichCommand or TreeRichGroup.
I think between tree_option() and context_options={"tree_option_names": ...}, that is plenty.
Truthfully, I don't anticipate most users will want help text completely overridden by the tree rendering. The tree rendering has upsides and downsides: the upside is it shows a lot of information; the downside is, well, it shows a lot of information.
I think many users would love having both a --tree and a --help, but not just one or the other. Hence, the suggested API.
In the few cases of users who want --help to be overridden by the tree rendering, and to ignore normal help text entirely, they have not one but two simple, intuitive, one-liner ways to do it with the rest of the API:
import rich_click as click
@click.group()
+@click.tree_option("--help")
def cli():
...or (passes down to subcommands as well):
import rich_click as click
+@click.group(context_settings={"tree_option_names": ["--help"]})
def cli():
...(Note, above does not 100% work with your current code, I believe? But that will be something we want working)
I think I understand why you did this approach, as it is 1. a port from your own library and 2. an easy way to patch the rich-click CLI. For the 2nd point, if that is tricky, then don't worry about it and I can take care of it, although you can take a stab at it. If you want to take a stab at it, see how I implement the HTML and SVG output stuff. That would be similar to how rich-click --tree ... works.
There was a problem hiding this comment.
Agreed, your suggested ways of switching, i.e. tree_option() and context_settings, are preferable.
I just didn't get this to work.
|
Hi, Some slightly bad news. I don't think we can get this in for 1.9. There's just too much work to do cleaning up 1.9 and getting this ready will take up too much additional time. But, Thank you a ton for your work on this. Please keep this PR open and we'll work on integrating it at some point in the near to medium term future, and it will get its own release. |
f9e9f13 to
6bb7846
Compare


First concept for tree view, bit rough still.