feat!: refactor to make click optional, support argparse, and ... #38
Conversation
fresh2dev
force-pushed
the
make-generic
branch
2 times, most recently
from
7469633
to
17de91b
Compare
| @@ -43,7 +43,7 @@ def on_mount(self) -> None: | |||
| getattr(schema.parent, "name", "No parent"), | |||
| ), | |||
| (Text("Subcommands", style="b"), list(schema.subcommands.keys())), | |||
| (Text("Group", style="b"), schema.is_group), | |||
| (Text("Group", style="b"), bool(schema.subcommands)), | |||
There was a problem hiding this comment.
A command is a group if it has subcommands; i.e.:
is_group = bool(schema.subcommands)
|
|
||
|
|
||
| class CommandTree(Tree[CommandSchema]): | ||
| COMPONENT_CLASSES = {"group"} | ||
|
|
||
| def __init__(self, label: TextType, cli_metadata: dict[CommandName, CommandSchema], command_name: str): | ||
| def __init__(self, label: TextType, cli_metadata: dict[CommandName, CommandSchema]): |
There was a problem hiding this comment.
The exclusion of a specific command-name can be handled in each parsers "introspector". See:
| label.stylize(group_style) | ||
| label.append(" ") | ||
| label.append("group", "dim i") | ||
| group_style = self.get_component_rich_style("group") |
There was a problem hiding this comment.
If a command has subcommands, it is a group.
| @@ -127,15 +127,17 @@ def compose(self) -> ComposeResult: | |||
| # If there are N defaults, we render the "group" N times. | |||
| # Each group will contain `nargs` widgets. | |||
| with ControlGroupsContainer(): | |||
| if not argument_type == click.BOOL: | |||
| if argument_type is not bool: | |||
There was a problem hiding this comment.
Use Python type over click-specific type.
| yield Label(label, classes="command-form-label") | ||
|
|
||
| if isinstance(argument_type, click.Choice) and multiple: | ||
| if schema.choices and multiple: |
There was a problem hiding this comment.
Use Python type over click-specific type.
trogon/widgets/parameter_controls.py
Outdated
| @@ -187,7 +189,7 @@ def compose(self) -> ComposeResult: | |||
| # If it's a multiple, and it's a Choice parameter, then we display | |||
| # our special case MultiChoice widget, and so there's no need for this | |||
| # button. | |||
| if multiple or nargs == -1 and not isinstance(argument_type, click.Choice): | |||
| if multiple or nargs == -1 and not schema.choices: | |||
There was a problem hiding this comment.
Use Python type over click-specific type.
trogon/widgets/parameter_controls.py
Outdated
| if isinstance(parameter_type, click.Tuple) | ||
| else [parameter_type] | ||
| ) | ||
| parameter_types = [parameter_type] * schema.nargs if schema.nargs > 1 else [parameter_type] |
There was a problem hiding this comment.
Use Python type over click-specific type.
Instead of relying on click-specific tuple types, let's determine the quantity of input boxes to create by referring to schema.nargs.
| else: | ||
| return self.make_text_control | ||
|
|
||
| return self.make_text_control |
There was a problem hiding this comment.
This is much shorter, and roughly equal, but removes respect for click-specific types like IntRange.
trogon/widgets/parameter_controls.py
Outdated
| @@ -401,26 +381,17 @@ def make_choice_control( | |||
| @staticmethod | |||
| def _make_command_form_control_label( | |||
| name: str | list[str], | |||
| type: click.ParamType, | |||
| type: Type[Any], | |||
There was a problem hiding this comment.
Use Python type over click-specific type.
| names = Text(" / ", style="dim").join([Text(n) for n in names]) | ||
| text = Text.from_markup( | ||
| f"{names}[dim]{' multiple' if multiple else ''} <{type.__name__}>[/] {' [b red]*[/]required' if is_required else ''}" | ||
| ) |
There was a problem hiding this comment.
This is much shorter, and roughly equal, but removes respect for click-specific types like IntRange.
|
|
||
| return app | ||
|
|
||
| return decorator |
trogon/trogon.py
Outdated
| self.post_run_command = event.command_data.to_cli_args(include_root_command) | ||
| self.post_run_command = event.command_data.to_cli_args( | ||
| include_root_command=False | ||
| ) |
There was a problem hiding this comment.
This was causing an issue in my tests, and I could not think of a single use-case that would need to include the root-command name; it inherently doesn't have a name. 🤷
With this consistently set to False, it proved to work fine for all examples, existing and new.
| # update PATH to include current working dir. | ||
| env: dict[str, str] = os.environ.copy() | ||
| env["PATH"] = os.pathsep.join([os.getcwd(), env["PATH"]]) | ||
| os.execvpe(program_name, arguments, env) |
There was a problem hiding this comment.
Add the current working dir to PATH, so that a single executable script can be discovered by and invoked by Trogon.
| app_version=self.app_version, | ||
| is_grouped_cli=self.is_grouped_cli, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
CommandBuilder now accepts the schemas directly, rather than inferring schemas from a given CLI.
| schema.parent = root_schema | ||
| root_schema.subcommands[schema.name] = schema | ||
|
|
||
| return cls(schemas, **kwargs) |
There was a problem hiding this comment.
This helper method allows for a Trogon instance to be instantiated from a list of schemas, where the 1st schema is the "root" schema, and subsequent schemas are its children.
There was a problem hiding this comment.
I may be misunderstanding this, but since CommandSchemas form a tree structure, is the list necessary here? If the subsequent schemas are just the children of the root, wouldn't they just be root.subcommands.values()?
| self.post_run_command: list[str] = [] | ||
| self.is_grouped_cli = isinstance(cli, click.Group) | ||
| self.command_schemas = command_schemas | ||
| self.is_grouped_cli = any(v.subcommands for v in command_schemas.values()) |
There was a problem hiding this comment.
This is a "grouped" CLI if any of commands have subcommands.
| click_context: click.Context | None = None, | ||
| command_schemas: dict[CommandName, CommandSchema], | ||
| app_name: str | None, | ||
| app_version: str | None = None, |
There was a problem hiding this comment.
Trogon now accepts the schemas directly, rather than inferring schemas from a given CLI. It is the responsibility of the various "introspectors" to do the reverse-engineering and provide schemas to Trogon.
| click_app_name: str, | ||
| command_name: str, | ||
| command_schemas: dict[CommandName, CommandSchema], | ||
| app_name: str, |
There was a problem hiding this comment.
Renamed click_app_name to just app_name throughout.
| cli: click.BaseCommand, | ||
| click_app_name: str, | ||
| command_name: str, | ||
| command_schemas: dict[CommandName, CommandSchema], |
There was a problem hiding this comment.
CommandBuilder now accepts the schemas directly, rather than inferring schemas from a given CLI.
| if sys.version_info >= (3, 8): | ||
| from importlib import metadata | ||
| else: | ||
| import importlib_metadata as metadata |
There was a problem hiding this comment.
Just to make IDEs a little happier.
|
|
||
|
|
||
| @dataclass | ||
| class OptionSchema(ArgumentSchema): |
There was a problem hiding this comment.
OptionSchema is a superset of ArgumentSchema, where ArgumentSchema is simply a positional argument, and OptionSchema is an argument with flags.
| for argument_value in this_arg_values: | ||
| if argument_value != ValueNotSupplied(): | ||
| args.append(argument_value) | ||
|
|
There was a problem hiding this comment.
Positional argument values go before non-positional argument ("option") values.
| for argument_value in this_arg_values: | ||
| if argument_value != ValueNotSupplied(): | ||
| args.append(argument_value) | ||
|
|
There was a problem hiding this comment.
Positional argument values go before non-positional argument ("option") values.
trogon/run_command.py
Outdated
| args.append(option_name) | ||
| # without multi-value (default): -u x -u y -u z | ||
| # with multi-value: -u x y z | ||
| if i == 0 or not option.option_schema.multi_value: |
There was a problem hiding this comment.
If multiple, this arg accepts multiple values and the resulting CLI command will look like this: -x 1 -x 2 -x 3
If multi_value, then multiple is inherently True, and the resulting CLI command will look like: -x 1 2 3
trogon/introspect.py
Outdated
There was a problem hiding this comment.
This file was split, placing general logic into schemas.py and click-specific logic into click.py.
There was a problem hiding this comment.
The contents of this file are an extract from introspect.py.
| @@ -2,3 +2,4 @@ | |||
| PACKAGE_NAME = "trogon" | |||
| TEXTUAL_URL = "https://github.com/textualize/textual" | |||
| ORGANIZATION_NAME = "T" | |||
| DEFAULT_COMMAND_NAME='tui' | |||
There was a problem hiding this comment.
This is referenced by any "introspectors" that want to use it.
There was a problem hiding this comment.
One thing I noticed was that when one of the values of a multi-value param is a "choice" type, the "Select" widget is not rendered (it seems to be defaulting to the text input widget)
For example: type=(str, click.Choice(["1", "2", "3"]))
My expectation would be (and this is how it works on main):
Good catch. My goal is to retain existing behavior. This one is tricky because an instantiated |
|
I restored the original behavior -- while remaining click-independent -- in this commit: This also addresses a bug that allows this to happen for multiple-choice parameters (both in main and in this fork):
|
fresh2dev
changed the title
This adds `read_only` and `placeholder` to the ArgumentSchema, which enables other use-cases as well.
Results in far fewer calls to `to_cli_args()`
Allow `ArgumentSchema` to contain one or more types. Define `ChoiceSchema` to allow choice-types in a sequence of types; used in place of `click.Choice`.
|
@darrenburns, any more feedback? |
|
Nevermind... |
|
@fresh2dev Darren has been very focused on a particular new feature of Textual recently, and we're just coming out of a period of holidays and an office move; on top of that we don't always get to our keyboards over the weekend. It can take a wee while to catch up on everything. |
|
@fresh2dev Please don’t take our slowness to respond as a lack of interest! |
|
Glad to hear that... TBH, a month without significant feedback -- here or on Discord -- did lead me to believe that there wasn't much interest. Given the potentially profound implications of this change, I was expecting a bit more engagement, but I understand that y'all have a lot of moving priorities. I sincerely admire the work y'all are putting in -- and the solutions you've delivered -- to elevate the command-line experience. All the time twiddling my thumbs on this PR gave room for some ideas to grow. Despite the fact that this PR was large, I did my best to limit my scope and changes to make it digestible and familiar enough. Once I became convinced this wasn't going anywhere, I unleashed my urges on a Trogon fork argparse-tui and have learned how argparse support is even more advantageous than I originally envisioned. Very eager to spruce up the docs and share more with your team in the near future. |
|
Coincidentally just stumbled across this post from @willmcgugan, referenced in an article currently on the HackerNews front page. https://textual.textualize.io/blog/2023/07/29/pull-requests-are-cake-or-puppies/ My work here is quite the puppy, and as puppies do, it's growin' fast! Very eager to show y'all the tricks this bitch is capable of 😂 |
make
clickoptionaladd introspection for argparse parser.
add support for manually constructing schemas for the TUI
add examples for
typeradd examples for
yapxadd examples for
mykeremove support for custom click types, e.g.,
IntRange,FloatRangepositional arguments come before keyword arguments in the generated command
ability to join list arguments values like this:
-x 1 -x 2 -x 3(default), or like this:-x 1 2 3(multi-value)omit commands decorated with
click.command(hidden=True)omit parameters decorated with
click.option(hidden=True)add these properties to
ArgumentSchema:read_only,placeholder, andsensitive. This enables:click.option(prompt=True, prompt_required=True)click.option(hide_input=True)Example of redaction of sensitive values:
https://asciinema.org/a/0aa5accX2QcN2D8oFwyCYty69
TODO: update README