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

Extract CLI crate and add shell completion and man page #42

Merged
merged 12 commits into from
Nov 13, 2023

Conversation

mrgalopes
Copy link
Contributor

Closes #27.

Adapted from the main typst repo. It only outputs the completions and the man page when the cli feature is enabled. A downside is that it always fetches and compiles the dependencies. Any suggestions are appreciated. 😄

@laurmaedje
Copy link
Member

I think having clap as a build dependency unconditionally is unfortunate. If we want to do this, maybe the CLI should be extracted into its own crate like in Typst.

cc: @reknih

@LaurenzV
Copy link
Collaborator

Yeah, I actually also extracted it when I did the rewrite initially, but I didn't know whether that was what you wanted so I ended up reverting to using a cli feature. I also think having them separate would be nicer.

@laurmaedje
Copy link
Member

Originally, I liked this setup, but I've come to prefer the more flexible two-crate setup since.

@mrgalopes
Copy link
Contributor Author

Yeah, extracting the CLI into its own crate should be nicer, you're right. I will update the PR in the following days with this setup. Thank you for the feedback!

@mrgalopes mrgalopes force-pushed the feature/shell-completion branch from 3b92f83 to 9e3056e Compare October 27, 2023 11:16
@mrgalopes
Copy link
Contributor Author

I separated the crates inspired by the structure of typst, and also rebased on main. Let me know what you think.

Copy link
Collaborator

@LaurenzV LaurenzV left a comment

Choose a reason for hiding this comment

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

Looks good to me in general, just a few nitpicks.

Also, I don't know anything about build scripts, so I will leave that @laurmaedje.

But could you maybe explain what this now makes possible which wasn't possible before? I tried to install using cargo install --path crates/svg2pdf-cli and then type svg2pdf --d and autocomplete to get --dpi, but it doesn't seem to work. Am I missing something?

Cargo.toml Outdated
@@ -1,47 +1,15 @@
[workspace]
members = ["tests"]
members = ["crates/*", "tests"]
default-members = ["crates/svg2pdf"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would change this to default-members = ["crates/typst-cli"], since the cli is what you want to execute by default when running cargo run in the root directory.

Comment on lines 26 to 29
image = { version = "0.24", default-features = false, features = [
"jpeg",
"png",
"gif",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess that's personal preference, but maybe better to keep everything in a single line? Not sure about this one, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a TOML formatter turned on for VSCode which did this, but I can revert it if you prefer, no problem.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer single lines, too.

@mrgalopes
Copy link
Contributor Author

mrgalopes commented Nov 6, 2023

Apologies for the delay. I've been looking for alternatives to add the completions but couldn't find any that were ideal. The way that this was done mirrors the typst-cli, where the nix flake installs the completion after the build. Without nix, this would have to be done manually. From what I could find, crates.io doesn't let you install arbitrary files, like completion files. This is how I've done for zsh:

  1. Update the .zshrc to include a folder that looks for completions and to autoload them
fpath+=~/.zfunc

autoload -Uz compinit
compinit
  1. Copy the completion artifact corresponding to zsh and bash to the ~/.zfunc folder, and restart the shell
cp target/release/artifacts/_svg2pdf ~/.zfunc

Comment on lines 26 to 29
image = { version = "0.24", default-features = false, features = [
"jpeg",
"png",
"gif",
Copy link
Member

Choose a reason for hiding this comment

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

I prefer single lines, too.

[build-dependencies]
clap = { version = "4.4.2", features = ["derive", "string"] }
clap_complete = "4.4.3"
clap_mangen = "0.2.14"
Copy link
Member

Choose a reason for hiding this comment

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

Please add newlines at the end of files where they are missing.

use clap_complete::{generate_to, Shell};

mod args {
include!("src/cli.rs");
Copy link
Member

Choose a reason for hiding this comment

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

I think the name args.rs (like in Typst) is clearer.

crates/svg2pdf-cli/Cargo.toml Show resolved Hide resolved
@laurmaedje laurmaedje changed the title Add shell completion and man page Extract CLI crate and add shell completion and man page Nov 13, 2023
@laurmaedje laurmaedje merged commit a405a57 into typst:main Nov 13, 2023
2 checks passed
@laurmaedje
Copy link
Member

Thank you!

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.

[feature] Shell completion
3 participants