Add nested task subgroup to file repository #110
Conversation
mdellweg
force-pushed
the
resource_tasks
branch
from
0ef57e6 to
5b6b57f
Compare
|
Code lgtm. Is this waiting on a test? |
I thought both a test as well as some more thought into design of the user facing command structure. |
|
OK,i just realized, that it suffers from the interspersed parameter syndrom. |
mdellweg
force-pushed
the
resource_tasks
branch
from
5b6b57f to
d54f07a
Compare
|
I wonder if this pattern might be applicable to #132 |
Maybe. We should follow a common design pattern here i think to keep the command structure consistent. |
mdellweg
force-pushed
the
resource_tasks
branch
2 times, most recently
from
48f60f0 to
a732e35
Compare
pulpcore/cli/file/repository.py
Outdated
| @@ -60,6 +70,9 @@ def repository(ctx: click.Context, pulp_ctx: PulpContext, repo_type: str) -> Non | |||
|
|
|||
|
|
|||
| lookup_options = [href_option, name_option] | |||
| nested_lookup_options = [ | |||
| click.option("--repository", callback=_repository_callback, expose_value=False) | |||
| ] | |||
There was a problem hiding this comment.
I am wondering, if this kind of lookup should be used for the version and label command groups too.
There was a problem hiding this comment.
It might help to see some code. At least for labels, I can't visualize a benefit.
There was a problem hiding this comment.
The problem is, that we cannot use --name and --href here because the TaskContext would ingest them.
So we are left with --repository and maybe --repository-href. For label it would be for the sake of consistency.
pulpcore/cli/file/repository.py
Outdated
| @@ -32,6 +33,15 @@ | |||
| _ = gettext.gettext | |||
|
|
|||
|
|
|||
| def _repository_callback( | |||
| ctx: click.Context, param: click.Parameter, value: Optional[str] | |||
| ) -> Optional[Union[str, PulpEntityContext]]: | |||
There was a problem hiding this comment.
Seems like it's simpler to use Union[str, PulpEntityContext, None] here. Also, I'm not sure how this would return a PulpEntityContext?
mdellweg
force-pushed
the
resource_tasks
branch
from
a732e35 to
715146b
Compare
mdellweg
force-pushed
the
resource_tasks
branch
from
715146b to
c52ec86
Compare
mdellweg
force-pushed
the
resource_tasks
branch
from
c52ec86 to
14cd8c4
Compare
|
Does this keep the lookup options on other label subcommands (e.g. |
Yes, in the end its meant to use the same structure everywhere. |
No description provided.