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

Generate shell completions with clap during build #327

Merged
merged 8 commits into from
Oct 3, 2018

Conversation

davideGiovannini
Copy link
Contributor

Auto-generate shell-completion files via clap #115

Copy link
Owner

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

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

This looks great, thank you very much!

We should also ship these files with each release (see e.g. https://github.com/sharkdp/fd/blob/56b6389dac945df313ce54072fcc01b91211a76c/ci/before_deploy.bash#L41-L43)

Cargo.toml Outdated
@@ -43,3 +44,6 @@ features = []

[dev-dependencies]
tempdir = "0.3"

[build-dependencies]
clap = "2.32.0"
Copy link
Owner

Choose a reason for hiding this comment

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

I think we should use "2.32" to be consistent with the dependency above.

build.rs Outdated

fn main() {

let outdir = match std::env::var_os("OUT_DIR") {
Copy link
Owner

Choose a reason for hiding this comment

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

Can we use the same mechanism as in fd here:

let outdir = std::env::var_os("SHELL_COMPLETIONS_DIR").or(std::env::var_os("OUT_DIR"));

See sharkdp/fd#121

It would also require us to use fs::create_dir_all:
sharkdp/fd@3104729

@@ -38,6 +38,11 @@ pack() {
cp LICENSE-MIT "$tempdir/$package_name"
cp LICENSE-APACHE "$tempdir/$package_name"

# various autocomplete
cp target/"$TARGET"/release/build/"$PROJECT_NAME"-*/out/"$PROJECT_NAME".bash "$tempdir/$package_name/autocomplete/${PROJECT_NAME}.bash-completion"
Copy link
Owner

Choose a reason for hiding this comment

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

We need to make sure that the target directory exists.

Copy link
Owner

@sharkdp sharkdp Oct 1, 2018

Choose a reason for hiding this comment

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

Also, it would be great to do this for Windows deployments as well (in the before_deploy.ps1 script). I think we don't need a subfolder in case of the Windows archive.

Copy link
Contributor Author

@davideGiovannini davideGiovannini Oct 1, 2018

Choose a reason for hiding this comment

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

I'm not familiar with how it should work on Windows.

Should it be something like this ?

Copy-Item "$SRC_DIR\target\release\build\bat-*\out\_bat.ps1" '.\autocomplete'

Copy link
Owner

Choose a reason for hiding this comment

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

I guess just

Copy-Item $SRC_DIR\target\release\build\bat-*\out\_bat.ps1 '.\'

@sharkdp
Copy link
Owner

sharkdp commented Oct 1, 2018

Travis is currently failing due to some formatting issues. You can cargo +nightly install rustfmt-nightly and then run cargo fmt from the repository folder. I can also do this for you, if you want.

@davideGiovannini
Copy link
Contributor Author

You can cargo +nightly install rustfmt-nightly and then run cargo fmt from the repository folder.

is this the same as running cargo fmt installed with rustup component add rustfmt-preview ?
It's changing the formatting of files that I did not change

        modified:   src/assets.rs
        modified:   src/diff.rs
        modified:   src/printer.rs

Do wildcards work inside quotes?

Uhm probably not.
I'm removing the quotes

@sharkdp
Copy link
Owner

sharkdp commented Oct 1, 2018

Hm, I'm not sure, but there might very well be a change in the format between this and rustfmt-nightly (which we currently use to enforce the style guide). This is unfortunate, but apparently there will be a 1.0 release soon which should stabilize the format.

@sharkdp sharkdp merged commit 0d71968 into sharkdp:master Oct 3, 2018
@sharkdp
Copy link
Owner

sharkdp commented Oct 3, 2018

Thank you very much!!

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