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

feat: add shell completion generation #1775

Merged
merged 10 commits into from
Oct 11, 2023

Conversation

NickyMeuleman
Copy link
Contributor

@NickyMeuleman NickyMeuleman commented Oct 6, 2023

This adds the ability to generate shell completions to the CLI.

While this PR is not in an ideal form yet, I'd be more than happy to work together to make it fit the used patterns in this repo.
I tried following them, but ran into trouble because the execute method for each command did not get a Command passed, which this shell completions generator needs.

Here is an example of me using the completions this generates:
https://asciinema.org/a/dsXad2IayVyXJFHKX0GjvRhjH

@loewenheim
Copy link
Contributor

This is very cool, thank you for the contribution! I'll get back to you on Monday to work out the kinks.

src/commands/mod.rs Outdated Show resolved Hide resolved
@loewenheim
Copy link
Contributor

loewenheim commented Oct 9, 2023

Another thing—did you intentionally move use statements around in 57890f1? I'd rather they stay as they were.

ETA: You will also need to fix the command_help test. The easiest way to do this is to run TRYCMD=overwrite cargo test. This will update all test snapshots with the new output. You can then check that output and commit it.

Other than that, this is ready to merge, I think.

@NickyMeuleman
Copy link
Contributor Author

Ah, I turned on format-on-save before making that commit, that must have reordered those use statements.
Put them back, and updated the snapshot file!

Do I need to write some docs for this also?

Thanks for the help and patience (with my not knowing how a CLI actually works)

@NickyMeuleman
Copy link
Contributor Author

NickyMeuleman commented Oct 10, 2023

I saw the Windows test failed, because they are also missing an update to the snapshot tests.

So I tried to set up this repo on a Windows machine, but 2 other tests failed there on the main branch. And curiously, those tests didn't fail in CI, but only when running cargo test locally on the Windows machine.

@NickyMeuleman
Copy link
Contributor Author

The errors happened because I used regular Windows cmd, when I used git bash, everything worked.
I updated the Windows snapshot test that was failing, all checks should pass now.

@loewenheim loewenheim merged commit e16f636 into getsentry:master Oct 11, 2023
@loewenheim
Copy link
Contributor

Merged! Thank you very much for the contribution!

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.

2 participants