Add fish description support #6
Conversation
pfgray
force-pushed
the
add_fish_description_support
branch
from
1d0dc34
to
550e4d1
Compare
src/app/mod.rs
Outdated
| for opt in &opts.options { | ||
| println!("{}", opt); | ||
| if print_descriptions { | ||
| match opt.desc.clone() { |
There was a problem hiding this comment.
You shouldn't need to clone, you should be able to use match opt.as_ref() { . That converts an &Option<T> to an Option<&T>, so will prevent moving opt.desc.
src/app/mod.rs
Outdated
| @@ -26,8 +26,17 @@ fn print_options(config_filename: &str, tokens: &[String], last_token: &str) -> | |||
| let options_finder = options_finder::OptionsFinder::new(result); | |||
| let opts = options_finder.options(last_token)?; | |||
|
|
|||
| let print_descriptions = std::env::var("TABRY_PRINT_DESCRIPTIONS").is_ok(); | |||
There was a problem hiding this comment.
Is there a reason you chose a env var over a flag? I think I would prefer a flag, although I can't explain why, it just somehow seems more suitable, so I could be convinced otherwise.
There was a problem hiding this comment.
That was just how I implemented it in the ruby version, I can switch to a flag, no problem!
src/engine/options_finder.rs
Outdated
| if value.starts_with(&self.prefix) { | ||
| // TODO get_or_insert_owned() in nightly would be ideal | ||
| self.options.insert(value.to_owned()); | ||
| self.options.insert(OptionResult { value: value.to_owned(), desc }); |
There was a problem hiding this comment.
Cloning desc in the caller instead of here means a clone happens regardless of whether value.starts_with(&self.prefix). It would be better to pass in a reference and do the clone here. In this case, probably passing in &s.description (i.e. the parameter would be desc: &Option<String>) would work.
It's a pity we have to clone the string here at all even if we aren't going to eventually show description (e.g. bash),. It could probably be avoided by passing in a parameter like needs_description. But that's modifying multiple levels of functions, and anyway, in the future, I may try to fix all this code so that we don't have to clone anything (if I can get the lifetimes to co-operate). For now, could you add a comment here like
// TODO: don't clone description here if we don't need it (probably will get fixed if we can not clone any part of the config)
3ea93fa
to
468bd80
Compare
Fish supports "descriptions" for completions, which are displayed to the right of the value offered for completion, to provide additional context for the user. To display a description for a completion, the description is printed after a tab character in the completion line. so, instead of just printing: ``` bar baz qux ``` for completions, we can print: ``` bar<TAB>The bar command baz<TAB>The baz command qux<TAB>The qux command ``` This commit adds support for fish descriptions to tabry. Instead of OptionsResults referencing a collection of `String`s, it now holds a collection of `OptionResult`s, which hold the value of the completion and an optional description. Tabry will print a description if it is present, and if the `TABRY_PRINT_DESCRIPTIONS` environment variable is set.
pfgray
force-pushed
the
add_fish_description_support
branch
from
468bd80
to
5708346
Compare
|
Thank you! |
Fish supports "descriptions" for completions, which are displayed to the
right of the value offered for completion, to provide additional context
for the user.
To display a description for a completion, the description is printed
after a tab character in the completion line.
so, instead of just printing:
for completions, we can print:
Which fish will display like this:
This commit adds support for fish descriptions to tabry.
Instead of OptionsResults referencing a collection of
Strings, it nowholds a collection of
OptionResults, which hold the value of thecompletion and an optional description.
Tabry will print a description if it is present, and if the
TABRY_PRINT_DESCRIPTIONSenvironment variable is set.