search

operator-framework / operator-courier

Build, verify and push operators
Apache License 2.0
41 stars 53 forks source link

Use argparse subparsers for subcommands #80

Closed csomh closed 5 years ago

csomh commented 5 years ago

Makes argument parsing more resilient, by handling subcommands with argparse provided and recommended subparsers, instead of the current custom manipulation of argv.

Removes the need to manually update and format the top-level help message: this is going to be composed from the help strings of the subcommands by argparse.

Fixes the issue of subcommand's usage string not displaying the subcommand.

Removes the need to explicitly check for incorrect subcommands.

Enables extending the CLI with top-level optional arguments, without the need of adjusting argv parsing.

openshift-ci-robot commented 5 years ago

Hi @csomh. Thanks for your PR.

I'm waiting for a operator-framework or openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
kevinrizza commented 5 years ago

What is the advantage of using subcommand versus the current implementation?

Also, https://github.com/operator-framework/operator-courier/issues/77 should be solved by a separate pr. Please do not co-mingle multiple changes in a single pull request.

csomh commented 5 years ago

Subparsers are the argparse provided way to handle subcommands. Switching to this was easier than figuring out the bug in the custom implementation, which was causing #77 or led to an incorrect usage text to be displayed for subcommands.

Sorry, the commit message didn't clarify this. Updated it, together with the initial PR comment.

kevinrizza commented 5 years ago

How do you know that there was a bug at all? It seems like the command has description output, and I'm not sure that your change fixed any bug at all. It seems more likely to me that the issue in question was describing a perceived lack of documentation about what the flag outputs (aside from the very basic description of the flag).

That could be a separate discussion, but I don't think this PR fixes the issue.

And, again, I still don't see what using subcommands contributes here. Is there some design limitation to the current implementation (aside from not using some specific sub library)? What does moving to using subcommand actually give us that we don't have already? If there is a technical argument to be made for them, I am certainly interested in hearing it.

csomh commented 5 years ago

Oh, ok, I see now. You're right.

MartinBasti commented 5 years ago

This PR is still valid (but must be updated), There were already several issues caused by not using subparsers.

csomh commented 5 years ago

Updated.

kevinrizza commented 5 years ago

/lgtm

MartinBasti commented 5 years ago

works for me