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

Enhance autocomplete generation #5612

Closed
wants to merge 15 commits into from
Closed

Enhance autocomplete generation #5612

wants to merge 15 commits into from

Conversation

crodas
Copy link
Contributor

@crodas crodas commented Feb 14, 2024

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.

@crodas crodas force-pushed the fig-io-autocomplete branch 2 times, most recently from c7ecc12 to 1d85dc9 Compare February 14, 2024 20:05
@crodas crodas force-pushed the fig-io-autocomplete branch 5 times, most recently from b56063e to 13afd94 Compare February 14, 2024 20:27
@crodas crodas self-assigned this Feb 14, 2024
@crodas crodas marked this pull request as ready for review February 15, 2024 20:15
@crodas crodas enabled auto-merge (squash) February 15, 2024 20:25
@crodas crodas force-pushed the fig-io-autocomplete branch 2 times, most recently from 98662ef to 6572be0 Compare February 15, 2024 20:58
Copy link
Member

@sdankel sdankel left a comment

Choose a reason for hiding this comment

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

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>

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment on lines +13 to +26
enum Target {
/// Bourne Again SHell (bash)
Bash,
/// Elvish shell
Elvish,
/// Friendly Interactive SHell (fish)
Fish,
/// PowerShell
PowerShell,
/// Z SHell (zsh)
Zsh,
/// Fig
Fig,
}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fig is not defined as a Shell.

forc-util/src/cli.rs Show resolved Hide resolved
Comment on lines 75 to 82
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(),
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this better now 3daa35f?

}
}

pub fn to_clap(&self) -> clap::App<'_> {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should just store the original clap command

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

impl ArgInfo {
pub fn to_clap(&self) -> clap::Arg<'_> {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should just store the original clap arg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

.get_possible_values()
.map(|x| {
x.iter()
.map(|x| PossibleValues {
Copy link
Member

Choose a reason for hiding this comment

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

Should we account for aliases and hidden args?

Comment on lines +139 to +119
let mut file = File::create(&forc_autocomplete_path).unwrap_or_else(|_| {
panic!("Cannot write to the autocomplete file in path {forc_autocomplete_path}")
});
Copy link
Member

Choose a reason for hiding this comment

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

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

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 guess we should just print a shell command they should to autoload the autocomplete definition

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

Choose a reason for hiding this comment

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

On mac, this is bash_profile

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


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");
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@sdankel sdankel left a comment

Choose a reason for hiding this comment

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

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

Copy link
Member

@sdankel sdankel left a comment

Choose a reason for hiding this comment

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

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

@sdankel
Copy link
Member

sdankel commented Feb 17, 2024

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.


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

@sdankel sdankel Feb 17, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member

@sdankel sdankel Feb 17, 2024

Choose a reason for hiding this comment

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

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}."

@crodas crodas force-pushed the fig-io-autocomplete branch 2 times, most recently from 6033734 to 914cded Compare February 19, 2024 15:24
@crodas crodas force-pushed the fig-io-autocomplete branch 3 times, most recently from a100f54 to a1f26d6 Compare February 22, 2024 16:12
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)
@crodas 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
This is the first part of #5612, which will be split into several smaller PRs
@crodas crodas mentioned this pull request Feb 26, 2024
7 tasks
crodas added a commit that referenced this pull request Apr 1, 2024
This is the first part of #5612, which will be split into several smaller PRs
crodas added a commit that referenced this pull request Apr 4, 2024
## 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.
@JoshuaBatty
Copy link
Member

Closing, not high priority.

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.

fig.io CLI auto-complete for plugins
4 participants