Skip to content
This repository has been archived by the owner on Oct 25, 2021. It is now read-only.

Add plugin subcommand to generate shell completions #173

Merged
merged 5 commits into from Sep 13, 2017

Conversation

chrisvittal
Copy link
Contributor

This is a followup to #168. Enough changes were made for me to warrant creating a new PR.

I implemented this as a tools subcommand after my attempt to make it an option of export had the unfortunate side effect of needing to have an artifact project initialized in order to use commands that did not need to have an associated project.

Currently the new subcommand has the following spec:

art tools [FLAGS] [OPTIONS] <value>

Where [FLAGS] are the standard -v, -q, -h, [OPTIONS] takes -o for output
file/directory in addition to --work-tree. Currently <value> can be
any of, 'bash-completions', 'fish-completions', 'zsh-completions', or
'ps-completions', each of which uses clap's completion generation
feature to create tab completion files for bash, fish, zsh, and
PowerShell respectively.
@vitiral
Copy link
Owner

vitiral commented Sep 10, 2017

I think this looks good. I'll test it out shortly.

I think I would like to change tools -> plugins. I plan to support plugins in the future. While shell completions are not strictly plugins (they won't be named art-<name>), they are external programs that artifact is interfacing with.

The changes would be:

  • change the name from tools -> plugins
  • The about would be "Command for integrating with external plugins, currently only supports exporting shell completions"
  • Arg::with_name("value") -> Arg::with_name("name") with an "about" of Plugin name.

@vitiral
Copy link
Owner

vitiral commented Sep 10, 2017

also, we've hit 1.0! I'm going to implement a beta channel to release this on while it is tested, stay tuned.

@vitiral
Copy link
Owner

vitiral commented Sep 10, 2017

also, sorry about the code churn here. You pushed this just as I was releasing 1.0, clearly I wasn't ready for contributions of new features!

@chrisvittal
Copy link
Contributor Author

Code churn is fine. I have a project that I was going to start soon, and this looked like a great tool for it, but the two things that really help make commandline tools useful for me are good shell completions and man pages. I figured it wouldn't be much work to add the completion generation, but man pages are harder.

@vitiral
Copy link
Owner

vitiral commented Sep 10, 2017

thanks for your patience. I've almost got the feature stream ready. I'll be pushing a PR and making sure it passes on travis. Should be ready in a few hours.

@vitiral
Copy link
Owner

vitiral commented Sep 10, 2017

ok, the beta channel has been added. Let me know if you hit any issues or have any confusion.

The functions run_beta and add_beta_cmds are the main ones to use, along with markeing your mod as #[cfg(feature = "beta")]

Pick up beta feature changes just enough so that a featureless cargo
check passes.
@chrisvittal
Copy link
Contributor Author

It looks like travis failed with an FailedConnectionException "package.elm-lang.org" 80.

Rerun?

@vitiral
Copy link
Owner

vitiral commented Sep 11, 2017 via email

Notable changes include the signature of run_beta, as some commands may
not need a project, and some commands need access to the io::Write
object passed to cmd.
Copy link
Owner

@vitiral vitiral left a comment

Choose a reason for hiding this comment

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

I can do the TODO, otherwise looks great! Thanks for doing this :)

}

#[cfg(not(feature = "beta"))]
/// run beta commands in the `[#cfg(feature = "beta")]` function
fn run_beta(_: &Project, _: &ArgMatches) -> Result<u8> {
fn run_beta<W>(_: Option<&Project>, _: &ArgMatches, _: &mut W) -> Result<u8>
Copy link
Owner

Choose a reason for hiding this comment

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

lol, Project does have to be an option... doesn't it!

Arg::with_name("name")
.required(true)
.possible_values(&[
"bash-completions",
Copy link
Owner

Choose a reason for hiding this comment

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

TODO: make these constants. I can tackle if you are busy.

static BASH_COMPLETIONS: &str = "bash-completions";
... etc

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 think we want const not static, with const the match expression can be

    BASH_COMPLETIONS => Plugin::Comp(Shell::Bash),
    FISH_COMPLETIONS => Plugin::Comp(Shell::Fish),
    ZSH_COMPLETIONS => Plugin::Comp(Shell::Zsh),
    PS_COMPLETIONS => Plugin::Comp(Shell::PowerShell),
    ...

as the const effectively inlines the constant. Otherwise we would need to do something like

    x if x == BASH_COMPLETIONS => Plugin::Comp(Shell::Bash),
    x if x == FISH_COMPLETIONS => Plugin::Comp(Shell::Fish),
    x if x == ZSH_COMPLETIONS => Plugin::Comp(Shell::Zsh),
    x if x == PS_COMPLETIONS => Plugin::Comp(Shell::PowerShell),
    ...

I'm not certain what this becomes, but it seems less elegant for serving the same purpose.

Incorporate review comments. Also fix miscellaneous clippy comments.
@chrisvittal chrisvittal changed the title Add tools subcommand to generate shell completions Add plugin subcommand to generate shell completions Sep 11, 2017
@vitiral vitiral merged commit 8e9984e into vitiral:master Sep 13, 2017
@vitiral
Copy link
Owner

vitiral commented Sep 13, 2017

Looks good, I didn't actually realize you could use static OR const for strings, much less that they actually meant different things.

I haven't tested this locally, but it's behind the beta flag so I'm going to merge it and test it out later. Thanks for putting this together.

@chrisvittal chrisvittal deleted the completions-2 branch September 13, 2017 02:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants