Skip to content

Conversation

@ofek
Copy link
Contributor

@ofek ofek commented Dec 7, 2025

This adds support for selecting subcommands as flags in their parent group which will be used when we soon migrate the static analysis checks/repo validations. Every command group in the chain will be able to run a subset of checks using flags.

image

@ofek ofek requested a review from a team as a code owner December 7, 2025 23:08
@pducolin
Copy link
Contributor

pducolin commented Dec 8, 2025

Is this meant to call multiple subcommands in one call? Why is it needed?

Copy link
Contributor

@Ishirui Ishirui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +135 to +150
helpers.dedent(
"""
import click
from dda.cli.base import dynamic_group
@dynamic_group(
invoke_without_command=True,
add_subcommand_selectors=True,
)
@click.pass_context
def cmd(ctx, **selections):
if not ctx.invoked_subcommand:
ctx.obj.display(" ".join(cmd.get_selected_subcommands(ctx, selections)))
"""
)
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Instead of writing at runtime, why not include a new file in testdata/ ? I've already used this pattern for some of my tests and imo it makes it easier to understand what is going on inside the test since you can open your test files with your IDE, get syntax highlighting etc.


return cmd_object

def get_params(self, ctx: DynamicContext) -> list[click.Parameter]: # type: ignore[override]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Add @override for clarity (maybe with the link to the source, as is added below)

Comment on lines +430 to +433
if self.__add_subcommand_selectors:
params.extend(
click.Option(param_decls=[f"--{command}"], is_flag=True) for command in self.list_commands(ctx)
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of specifying an option on the group to add all subcommands as flags, would it be possible to add an include_as_flag option on the subcommand itself ?

Within one group, we might not always want to have all subcommands available as flags.
It's also unclear to me how this will work if subcommands have flag arguments themselves ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, would it make sense to create an entirely new subclass of Group ? Considering the main usecase of dda check, it feels like the semantics of this new kind of command group are different enough that it would make sense.
Maybe something like this:

class FlagGroup(DynamicGroup):
   # Implement it such that all commands in this group automatically get registered as flags

   def cmd(...):
        # Here we could implement cmd such that a `FlagGroup` always implements the behavior in your example: running all subcommands sequentially by default, and only selecting some of them if flags have been passed

@Ishirui
Copy link
Contributor

Ishirui commented Dec 8, 2025

Is this meant to call multiple subcommands in one call? Why is it needed?

I assume this is because the main usecase is, for example, "run all linters", with the ability to select only some of them if we want.
But I agree that if this is the case, then the semantics of this feature are different enough from "standard" command groups to warrant defining an entirely new type of command group - see my review comment 😁

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 this pull request may close these issues.

4 participants