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 man pages and shell completions #683

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

donovanglover
Copy link

@donovanglover donovanglover commented Jul 30, 2024

Auto-generated with clap.

Note: May be ideal to replace some of the sub-commands with flags to decrease the amount of generated man pages.

Edit: Draft while I think of something better to do here

@donovanglover donovanglover force-pushed the feat/man-pages-shell-completions branch from 3813a38 to a6b1254 Compare July 30, 2024 18:34
@donovanglover donovanglover marked this pull request as draft August 1, 2024 15:35
@donovanglover
Copy link
Author

Upstream issue: clap-rs/clap#5337

Could add shell completions now and think about man pages later.

Long-term could auto-complete bar names and other information as well.

@donovanglover donovanglover force-pushed the feat/man-pages-shell-completions branch from a6b1254 to e329f3f Compare November 2, 2024 02:48
@donovanglover donovanglover marked this pull request as ready for review November 2, 2024 02:48
Copy link
Owner

@JakeStanger JakeStanger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work here, this will be a great QoL improvement to have. Looking like it's most of the way there already.

I'll be looking to merge this as part of the 0.17.0 release as 0.16.1 will be bugfixes only.

let man_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("target/man");
let mut buffer = Vec::default();

Man::new(cmd.clone()).render(&mut buffer).unwrap();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate this is just a build script, but I'd still prefer to keep unwraps out of the code pls. I'll be more lenient than usual towards using expect everywhere so long as it's clear why it shouldn't crash.

src/cli.rs Outdated
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for the restructuring here and moving handle_response? It looks like the diff has gone funny, so I'm assuming this isn't entirely intentional?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was to avoid unused code warnings from build.rs

@JakeStanger JakeStanger added this to the 0.17.0 milestone Nov 4, 2024
@donovanglover
Copy link
Author

Unfortunately I doubt I'll have time to work on this anytime soon but you're free to change anything as needed. Should also avoid duplicate code from src/ipc/commands.rs.

@JakeStanger
Copy link
Owner

No worries, thanks for the heads up. I'm happy to take it from here, appreciate the work you've done to get it this far.

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