Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow selecting required API version via --api-version #15

Merged
merged 2 commits into from
Nov 28, 2021

Conversation

imp
Copy link
Collaborator

@imp imp commented Nov 28, 2021

I have quite a lot of positive experience with srtuctopt, and clap 3 is essentially it, so I rewrote arg parsing to use Parser trait. Hope you don't mind.

Selecting required version - please have a look, I believe I got it right but would love to get feedback.

And the last one - macro_use is so Rust 2015, just couldn't help it :)

@clux
Copy link
Member

clux commented Nov 28, 2021

Hey, thanks for this! I think either clap / structopt makes sense. Is there any particular benefit you see from structopt over clap?

I did start out with clap 3 beta because I wanted dynamic auto-complete of arg1 via #6. If structopt uses clap 2 under the hood, I don't think it will support it, but maybe I am wrong.

I see you've also started doing bits from #3 in here, appreciate it!

@imp
Copy link
Collaborator Author

imp commented Nov 28, 2021

To the best of my knowledge clap 3 is essentially clap 2 + structopt. One visible advantage is that no *-beta.x versions appear in dependencies. Other than that should be identical. I don't believe there is any functionality gap,- I am fine with both.

As for generating autocompletion - the autocompletion code runs out of the kopium context AFAIK. That said it should perfectly ok to use kopium to provide required completion data for the autocomplete scripts.
I.e. possible crd names may be fed by kopium list-crd in order to enable auto-completion. The alternative is to use kubectl, however that creates unneeded dependency.

Also - I see you prefer --api-version to --version, which makes sense.

@imp imp changed the title Allow selecting required API version via --version Allow selecting required API version via --api-version Nov 28, 2021
@clux
Copy link
Member

clux commented Nov 28, 2021

Ok, I had a look at clap's generation and it's still not great. The dynamic completion support is still open: clap-rs/clap#1232 so no need to use a clap beta for now. Your changes with structopt simplifies the generation so let's go with this.

I don't have anything to really nit at in this PR, so happy to merge unless you want to do more of #3 here while you are at it.

Signed-off-by: Cyril Plisko <cyril.plisko@mountall.com>
Signed-off-by: Cyril Plisko <cyril.plisko@mountall.com>
@imp
Copy link
Collaborator Author

imp commented Nov 28, 2021

Alright, so structopt it is, squashed and rebased.

@clux clux mentioned this pull request Nov 28, 2021
3 tasks
@clux clux merged commit 26281b7 into kube-rs:main Nov 28, 2021
@clux
Copy link
Member

clux commented Nov 28, 2021

Nice. Merging this one while it's ready. Thank you.

@imp imp deleted the crd-version branch November 29, 2021 08:38
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.

None yet

2 participants