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

Add subcommand to generate completions, remove build.rs #369

Merged
merged 2 commits into from
Apr 25, 2021

Conversation

matthiasbeyer
Copy link
Contributor

As promised, this is the PR to change the CLI completion generation to not be in a build-time process, but rather in a subcommand for the binary itself.

Tell me what you think!


After working the first time with structopt, I must say it is way worse than I expected. Using plain clap just feels so much more powerful than structopts interface. Just as a note, not the concern of the zellij project of course.

Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
@a-kenji
Copy link
Contributor

a-kenji commented Apr 25, 2021

I personally am happy with it in general.
Maybe we want to put the completions in a file called setup.rs or put it in install.rs in order to not fill up the main too much?

cc @henil @TheLostLambda @qballer

Is this something we can merge soon, or do we need to change quite a few things in the build infrastructure for that? I am not sure how we have used the completions in the past.

After working the first time with structopt, I must say it is way worse than I expected. Using plain clap just feels so much more powerful than structopts interface. Just as a note, not the concern of the zellij project of course.

I have already lost a few hours to structopt weirdness. I think for now it is ok. We might want to or even need to switch to clap at some point, since structopt is limited in what it can derive. I personally am eager to try out v3 once it stabilizes.

@a-kenji
Copy link
Contributor

a-kenji commented Apr 25, 2021

Maybe we want to put the completions in a file called setup.rs or put it in install.rs in order to not fill up the main too much?

Ah, no I looked at the file again it should be totally fine for now.

@matthiasbeyer
Copy link
Contributor Author

I am using clapv3 beta for a few months now in one project at work and it is awesome.

I think the main won't get filled too much if you'd use the clap interface in a different way. That might be included in a refactor from structopt to clap, I guess.

@matthiasbeyer
Copy link
Contributor Author

Pushed a fixup commit for formatting, will rebase before merge.

Not sure about the test failure tho.

@a-kenji
Copy link
Contributor

a-kenji commented Apr 25, 2021

I am using clapv3 beta for a few months now in one project at work and it is awesome.

Nice!

Not sure about the test failure tho.

If I understand it correctly, sometimes the snapshot tests get confused, if they are too close to each other.

@matthiasbeyer
Copy link
Contributor Author

If I understand it correctly, sometimes the snapshot tests get confused, if they are too close to each other.

ah whut? How?

@h3nill
Copy link
Member

h3nill commented Apr 25, 2021

Is this something we can merge soon, or do we need to change quite a few things in the build infrastructure for that? I am not sure how we have used the completions in the past.

I am out of loop for sometime, so cannot say for sure but I think it should not require any big changes. Simply deleting build.rs should be fine.

@matthiasbeyer
Copy link
Contributor Author

Simply deleting build.rs should be fine.

That is done in this PR 😄

@a-kenji a-kenji merged commit d89e3e9 into zellij-org:main Apr 25, 2021
@a-kenji
Copy link
Contributor

a-kenji commented Apr 25, 2021

Awesome!

@matthiasbeyer
Copy link
Contributor Author

Yeah, that rebase was still pending. Either way... Thanks.

@a-kenji
Copy link
Contributor

a-kenji commented Apr 25, 2021

Oh, right. My bad!

a-kenji added a commit that referenced this pull request Apr 25, 2021
@matthiasbeyer matthiasbeyer deleted the clap-generate-shellcompletion branch April 26, 2021 12:52
This pull request was closed.
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.

3 participants