Enhance autocomplete generation #5612

crodas · 2024-02-14T19:53:38Z

This PR fixes #5474,

This PR enhances the autocomplete feature that is built-in ly forc. The first enhancement is to add support to generate a script for fig.

The second enhancement is to add support to let each plugin (automatically through cli_examples) dump its clap configuration. This configuration is shared to the main forc binary which creates a single autocomplete script for the requested shell that is also aware of every plugin. If a plugin uses cli_examples! macro this will automatically inherit this feature without any additional change.

~~The third improvement, still under development, is to install automatically the autocomplete feature instead of printing to the stdout (to address FuelLabs/fuelup#548)~~ (This feature has been added on c7015d0)

Description

Checklist

I have linked to any relevant issues.
I have commented my code, particularly in hard-to-understand areas.
I have updated the documentation where relevant (API docs, the reference, and the Sway book).
I have added tests that prove my fix is effective or that my feature works.
I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
I have requested a review from the relevant team or maintainers.

forc/src/cli/commands/completions.rs

sdankel

Could you add instructions on how to test this? I couldn't get autocompletions showing up after doing the following:

$ cargo install --path forc
$ cargo install --path forc-plugins/forc-crypto
$ forc completions
$ forc crypto <TAB>
$ forc-crypto <TAB>

sdankel · 2024-02-17T00:16:27Z

forc-util/src/cli.rs

+pub struct CommandInfo {
+    pub name: String,
+    #[serde(skip_serializing_if = "Option::is_none")]
+    #[serde(default)]


Is #[serde(default)] necessary for options?

Yes, otherwise it would error if the field is not set as null. With serde(default) the non-existence of the field is treated as None/null.

sdankel · 2024-02-17T00:25:28Z

forc/src/cli/commands/completions.rs

+enum Target {
+    /// Bourne Again SHell (bash)
+    Bash,
+    /// Elvish shell
+    Elvish,
+    /// Friendly Interactive SHell (fish)
+    Fish,
+    /// PowerShell
+    PowerShell,
+    /// Z SHell (zsh)
+    Zsh,
+    /// Fig
+    Fig,
+}


Do we need to redefine this, or can we just use clap_complete::Shell?

Fig is not defined as a Shell.

forc-util/src/cli.rs

sdankel · 2024-02-17T01:02:47Z

forc-util/src/cli.rs

+                possible_values: opt
+                    .get_possible_values()
+                    .map(|x| {
+                        x.iter()
+                            .map(|x| PossibleValues {
+                                name: x.get_name().to_owned(),
+                                help: x.get_help().unwrap_or_default().to_owned(),
+                            })
+                            .collect::<Vec<_>>()
+                    })
+                    .unwrap_or_default(),


nit: it might be nice to move this to a helper function

Is this better now 3daa35f?

sdankel · 2024-02-17T01:06:13Z

forc-util/src/cli.rs

+        }
+    }
+
+    pub fn to_clap(&self) -> clap::App<'_> {


Perhaps we should just store the original clap command

The original clap definition is defined in an external binary in a different namespace and it cannot be serialized with serde directly. If that was possible this PR would have been way shorter.

sdankel · 2024-02-17T01:06:30Z

forc-util/src/cli.rs

+}
+
+impl ArgInfo {
+    pub fn to_clap(&self) -> clap::Arg<'_> {


Perhaps we should just store the original clap arg

We cannot, as the clap struct cannot be serialized, it would have been much easier if that was the case.

sdankel · 2024-02-17T01:08:38Z

forc-util/src/cli.rs

+                    .get_possible_values()
+                    .map(|x| {
+                        x.iter()
+                            .map(|x| PossibleValues {


Should we account for aliases and hidden args?

sdankel · 2024-02-17T01:17:01Z

forc/src/cli/commands/completions.rs

+    let mut file = File::create(&forc_autocomplete_path).unwrap_or_else(|_| {
+        panic!("Cannot write to the autocomplete file in path {forc_autocomplete_path}")
+    });


We should definitely ask users before writing to their shell configuration. We should also tell the user what files are being written to.

I guess we should just print a shell command they should to autoload the autocomplete definition

sdankel · 2024-02-17T01:17:43Z

forc/src/cli/commands/completions.rs

+            "{}/.config/powershell/Microsoft.PowerShell_profile.ps1",
+            dir
+        )),
+        Target::Bash => Some(format!("{}/.bashrc", dir)),


On mac, this is bash_profile

Since this varies by OS, could we use a crate to get the right shell config file(s)?

sdankel · 2024-02-17T01:19:36Z

forc/src/cli/commands/completions.rs

+
+    if let Some(file_path) = user_shell_config {
+        // Update the user_shell_config
+        let file = File::open(&file_path).expect("Open the shell config file");


Does this cause a panic? What does this look like to the user?

sdankel

We definitely need to ask the user before modifying their shell config.

sdankel

We should also have some unit tests for this. This could be useful: https://docs.rs/completest/0.0.12/completest/

sdankel · 2024-02-17T02:14:43Z

I got it working by manually adding source /Users/sophiedankel/.forc.autocomplete to my ~/.bash_profile

It seems like there's an issue with autocompleting flags. For example, I would expect this: forc crypto --ve to give me the autocomplete suggestion: forc crypto --version but it doesn't.

It also displays it as <version> rather than --version

$ forc crypto
<help>          address         help            new-key         sha256
<version>       get-public-key  keccak256       parse-secret

I would also expect this: forc crypto v to suggest forc crypto --version as an autocomplete.

sdankel · 2024-02-17T02:19:01Z

forc/src/cli/commands/completions.rs

+
+        for line in reader.lines().map_while(Result::ok) {
+            if line.contains(&forc_autocomplete_path) {
+                println!("Forc completions is already installed");


println!("Forc completions are already installed at {shell_config_path}");

If the completions script has been updated, the user should not see this message. They should instead see something like "Forc completions were successfully updated at {forc_autocomplete_path}."

forc/src/cli/commands/completions.rs

This PR fixes #5474, This PR enhances the autocomplete feature that is built-in ly forc. The first enhancement is to add support to generate a script for [fig](https://fig.io). The second enhancement is to add support to let each plugin (automatically through cli_examples) dump its clap configuration. This configuration is shared to the main `forc` binary which creates a single autocomplete script for the requested shell that is also aware of every `plugin`. If a plugin uses `cli_examples!` macro this will automatically inherit this feature without any additional change. The third improvement, still under development, is to install automatically the autocomplete feature instead of printing to the stdout (to address FuelLabs/fuelup#548)

Co-authored-by: Sophie Dankel <47993817+sdankel@users.noreply.github.com>

Call forc_util::cli::register() explicitly instead of doing some dark magic behind through cli_examples macro

It is not desired for [fig.io](https://github.com/withfig/autocomplete/actions/runs/7978839646/job/21928919907?pr=2280)

This is the first part of #5612, which will be split into several smaller PRs

## Description This fixes #5474 This is the first part of #5612, which will be split into several smaller PRs ## Checklist - [x] I have linked to any relevant issues. - [x] I have commented my code, particularly in hard-to-understand areas. - [x] I have updated the documentation where relevant (API docs, the reference, and the Sway book). - [x] I have added tests that prove my fix is effective or that my feature works. - [x] I have added (or requested a maintainer to add) the necessary `Breaking*` or `New Feature` labels where relevant. - [x] I have done my best to ensure that my PR adheres to [the Fuel Labs Code Review Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md). - [x] I have requested a review from the relevant team or maintainers.

crodas force-pushed the fig-io-autocomplete branch 2 times, most recently from c7ecc12 to 1d85dc9 Compare February 14, 2024 20:05

kayagokalp reviewed Feb 14, 2024

View reviewed changes

forc/src/cli/commands/completions.rs Outdated Show resolved Hide resolved

crodas force-pushed the fig-io-autocomplete branch 5 times, most recently from b56063e to 13afd94 Compare February 14, 2024 20:27

crodas self-assigned this Feb 14, 2024

crodas marked this pull request as ready for review February 15, 2024 20:15

crodas force-pushed the fig-io-autocomplete branch from ca1e203 to 966e641 Compare February 15, 2024 20:22

crodas enabled auto-merge (squash) February 15, 2024 20:25

crodas force-pushed the fig-io-autocomplete branch 2 times, most recently from 98662ef to 6572be0 Compare February 15, 2024 20:58

sdankel reviewed Feb 17, 2024

View reviewed changes

sdankel requested changes Feb 17, 2024

View reviewed changes

sdankel reviewed Feb 17, 2024

View reviewed changes

forc/src/cli/commands/completions.rs Outdated Show resolved Hide resolved

sdankel reviewed Feb 17, 2024

View reviewed changes

forc/src/cli/commands/completions.rs Outdated Show resolved Hide resolved

crodas force-pushed the fig-io-autocomplete branch 2 times, most recently from 6033734 to 914cded Compare February 19, 2024 15:24

crodas mentioned this pull request Feb 21, 2024

fig.io CLI auto-complete for plugins #5474

Closed

crodas force-pushed the fig-io-autocomplete branch 3 times, most recently from a100f54 to a1f26d6 Compare February 22, 2024 16:12

crodas added 3 commits February 22, 2024 15:02

Use target instead of shell

b443ae7

Add support for possible_values

978adad

crodas and others added 11 commits February 22, 2024 15:02

Install completions scripts instead printing to the stdout

3d188e5

Fix clippy issues

f290c25

Add comments

385c660

Apply suggestions from code review

02b522e

Co-authored-by: Sophie Dankel <47993817+sdankel@users.noreply.github.com>

Minor changes

9065871

Minor change

54d5fff

Minor changes

b4bc1d9

Show activation instruciton instead of updating the file

fb903ae

Better plugin name

9a2820d

WIP 5

13119c7

Less magic

9c2044f

Call forc_util::cli::register() explicitly instead of doing some dark magic behind through cli_examples macro

crodas force-pushed the fig-io-autocomplete branch from a1f26d6 to 9c2044f Compare February 22, 2024 18:02

Remove trailing dot

7ff448d

It is not desired for [fig.io](https://github.com/withfig/autocomplete/actions/runs/7978839646/job/21928919907?pr=2280)

crodas marked this pull request as draft February 26, 2024 16:55

auto-merge was automatically disabled February 26, 2024 16:55
Pull request was converted to draft

crodas added a commit that referenced this pull request Feb 26, 2024

Add support for fig completions

5b5c705

This is the first part of #5612, which will be split into several smaller PRs

crodas mentioned this pull request Feb 26, 2024

Add support for fig completions #5669

Merged

7 tasks

crodas added a commit that referenced this pull request Apr 1, 2024

Add support for fig completions

70cc82d

This is the first part of #5612, which will be split into several smaller PRs

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance autocomplete generation #5612

Enhance autocomplete generation #5612

crodas commented Feb 14, 2024 •

edited

sdankel left a comment •

edited

sdankel Feb 17, 2024

crodas Feb 17, 2024

sdankel Feb 17, 2024

crodas Feb 17, 2024

sdankel Feb 17, 2024

crodas Feb 19, 2024

sdankel Feb 17, 2024

crodas Feb 17, 2024

sdankel Feb 17, 2024

crodas Feb 19, 2024

sdankel Feb 17, 2024

sdankel Feb 17, 2024

crodas Feb 17, 2024

sdankel Feb 17, 2024

sdankel Feb 17, 2024

sdankel left a comment

sdankel left a comment •

edited

sdankel commented Feb 17, 2024 •

edited

sdankel Feb 17, 2024 •

edited

sdankel Feb 17, 2024 •

edited

Enhance autocomplete generation #5612

Are you sure you want to change the base?

Enhance autocomplete generation #5612

Conversation

crodas commented Feb 14, 2024 • edited

Description

Checklist

sdankel left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdankel left a comment

Choose a reason for hiding this comment

sdankel left a comment • edited

Choose a reason for hiding this comment

sdankel commented Feb 17, 2024 • edited

sdankel Feb 17, 2024 • edited

Choose a reason for hiding this comment

sdankel Feb 17, 2024 • edited

Choose a reason for hiding this comment

crodas commented Feb 14, 2024 •

edited

sdankel left a comment •

edited

sdankel left a comment •

edited

sdankel commented Feb 17, 2024 •

edited

sdankel Feb 17, 2024 •

edited

sdankel Feb 17, 2024 •

edited