Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Conversation

@theonekeyg
Copy link
Contributor

Implement completion SubCommand for solana-cli, which outputs
completion script to stdout and exits the process. The script generation
handled completely by clap.

In order to implement the generation, one minor design change was
necessary regarding the creation of clap App.

Previously: One part of App initialization was in the app function,
and some other arguments and subcommands were added later directly in
the main function.

Now: The whole construction of App was moved to get_clap_app function.

P.S. I wasn't sure if constructing App separately had visual importance,
so both constructing parts are still separate in base_clap_app and
finalize_clap_app functions. But they sure could be one single function.

Problem

Absence of shell completions in solana-cli. (#8879 and #14005)

Summary of Changes

  • Added completion subcommand for solana-cli to generate completion scripts.
  • Tiny rework of App initialization.

Fixes #8879 and #14005

…and solana-labs#14005)

Implement `completion` SubCommand for solana-cli, which outputs
completion script to stdout and exits the process. The script generation
handled completely by clap.

In order to implement the generation, one minor design change was
necessary regarding the creation of clap `App`.

Previously: One part of App initialization was in the `app` function,
and some other arguments and subcommands were added later directly in
the `main` function.

Now: The whole construction of App was moved to `get_clap_app` function.

P.S. I wasn't sure if constructing App separately had visual importance,
so both constructing parts are still separate in `base_clap_app` and
`final_clap_app` functions. But they sure could be in one single
function.
@mergify mergify bot added the community Community contribution label Jul 27, 2021
@mergify mergify bot requested a review from a team July 27, 2021 23:12
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Thanks! I've been wanting this for a while.

I actually don't think there's a strong reason the base_ and finalize_ parts of the App have to remain separate anymore. Wdyt about adding a separate module to hold the app-construction function?

@t-nelson , can you take a look at this as well?

@CriesofCarrots CriesofCarrots requested a review from t-nelson July 28, 2021 01:16
t-nelson
t-nelson previously approved these changes Jul 28, 2021
Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

Looks great! I've been wanting these for awhile too

@t-nelson
Copy link
Contributor

I actually don't think there's a strong reason the base_ and finalize_ parts of the App have to remain separate anymore. Wdyt about adding a separate module to hold the app-construction function?

Agreed. Let's tie these up while they're getting shuffled around anyway

@mergify mergify bot dismissed t-nelson’s stale review July 28, 2021 06:38

Pull request has been modified.

@codecov
Copy link

codecov bot commented Jul 28, 2021

Codecov Report

Merging #18931 (ec21563) into master (eb6656a) will decrease coverage by 0.0%.
The diff coverage is 94.6%.

@@            Coverage Diff            @@
##           master   #18931     +/-   ##
=========================================
- Coverage    82.8%    82.8%   -0.1%     
=========================================
  Files         444      445      +1     
  Lines      126716   126846    +130     
=========================================
+ Hits       104971   105063     +92     
- Misses      21745    21783     +38     

Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Looks great, thank you so much!

@CriesofCarrots CriesofCarrots merged commit 9d0a937 into solana-labs:master Jul 28, 2021
mergify bot pushed a commit that referenced this pull request Jul 28, 2021
… (#18931)

* Auto-generate shell completions for solana-cli (issue #8879 and #14005)

Implement `completion` SubCommand for solana-cli, which outputs
completion script to stdout and exits the process. The script generation
handled completely by clap.

In order to implement the generation, one minor design change was
necessary regarding the creation of clap `App`.

Previously: One part of App initialization was in the `app` function,
and some other arguments and subcommands were added later directly in
the `main` function.

Now: The whole construction of App was moved to `get_clap_app` function.

P.S. I wasn't sure if constructing App separately had visual importance,
so both constructing parts are still separate in `base_clap_app` and
`final_clap_app` functions. But they sure could be in one single
function.

* Dereplicode match expr, fix clippy warning.

* Move clap App construction into separate module

Also join two parts of the construction into a single function

* Fix tests

* Apply rustfmt lints

(cherry picked from commit 9d0a937)
mergify bot added a commit that referenced this pull request Jul 28, 2021
… (#18931) (#18945)

* Auto-generate shell completions for solana-cli (issue #8879 and #14005)

Implement `completion` SubCommand for solana-cli, which outputs
completion script to stdout and exits the process. The script generation
handled completely by clap.

In order to implement the generation, one minor design change was
necessary regarding the creation of clap `App`.

Previously: One part of App initialization was in the `app` function,
and some other arguments and subcommands were added later directly in
the `main` function.

Now: The whole construction of App was moved to `get_clap_app` function.

P.S. I wasn't sure if constructing App separately had visual importance,
so both constructing parts are still separate in `base_clap_app` and
`final_clap_app` functions. But they sure could be in one single
function.

* Dereplicode match expr, fix clippy warning.

* Move clap App construction into separate module

Also join two parts of the construction into a single function

* Fix tests

* Apply rustfmt lints

(cherry picked from commit 9d0a937)

Co-authored-by: theonekeyg <[email protected]>
@brooksprumo brooksprumo mentioned this pull request Aug 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

community Community contribution

Projects

None yet

Development

Successfully merging this pull request may close these issues.

No tab completion in CLI

3 participants