Update fish completions #15174

razvanazamfirei · 2023-04-08T03:15:44Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew style with your changes locally?
Have you successfully run brew typecheck with your changes locally?
Have you successfully run brew tests with your changes locally?

Updating fish completions to remove expensive brew list calls

apainintheneck

This generally seems to be the approach taken with the Bash completions so seems like the right approach. It'd be nice to have some benchmarks though.

brew/completions/bash/brew

Lines 54 to 66 in c516870

    
           __brew_complete_installed_formulae() { 
        
             local cur="${COMP_WORDS[COMP_CWORD]}" 
        
             local installed_formulae 
        
             installed_formulae="$(command ls "${HOMEBREW_CELLAR:-$(brew --cellar)}" 2>/dev/null)" 
        
             while read -r line; do COMPREPLY+=("${line}"); done < <(compgen -W "${installed_formulae}" -- "${cur}") 
        
           } 
        
           __brew_complete_installed_casks() { 
        
             local cur="${COMP_WORDS[COMP_CWORD]}" 
        
             local installed_casks 
        
             installed_casks="$(command ls "$(brew --caskroom)" 2>/dev/null)" 
        
             while read -r line; do COMPREPLY+=("${line}"); done < <(compgen -W "${installed_casks}" -- "${cur}") 
        
           }

apainintheneck · 2023-04-09T17:10:10Z

Library/Homebrew/completions/fish.erb

+    else if test -n "$HOMEBREW_NO_INSTALL_FROM_API"
+        command ls -1 (brew --prefix)/Library/Taps/homebrew/homebrew-core
+    else if test -f "$__brew_cache_path/api/formula_names.txt"
+        command cat $__brew_cache_path/api/formula_names.txt
    else
+        # fallback on brew formulae if homebrew-core is not tapped


Will this ignore non-core formulae after this change?

Good question.

It would. brew formulae is actually decently fast so no need for this.

MikeMcQuaid

Great work so far!

Library/Homebrew/completions/fish.erb

MikeMcQuaid · 2023-04-10T11:55:30Z

Library/Homebrew/completions/fish.erb

+    else if test -n "$HOMEBREW_NO_INSTALL_FROM_API"
+        command ls -1 (brew --prefix)/Library/Taps/homebrew/homebrew-core
+    else if test -f "$__brew_cache_path/api/formula_names.txt"
+        command cat $__brew_cache_path/api/formula_names.txt
    else
+        # fallback on brew formulae if homebrew-core is not tapped


Good question.

razvanazamfirei · 2023-04-10T15:04:38Z

Library/Homebrew/completions/fish.erb

 end

 function __fish_brew_suggest_formulae_installed
-    brew list --formula
+    command ls -1 (brew --cellar)


❯ hyperfine 'brew list --formula' 'command ls -1 (brew --cellar)' --shell fish Benchmark 1: brew list --formula Time (mean ± σ): 473.1 ms ± 26.9 ms [User: 248.4 ms, System: 107.8 ms] Range (min … max): 443.6 ms … 542.5 ms 10 runs Benchmark 2: command ls -1 (brew --cellar) Time (mean ± σ): 10.0 ms ± 0.9 ms [User: 3.4 ms, System: 4.3 ms] Range (min … max): 8.7 ms … 14.2 ms 112 runs Summary 'command ls -1 (brew --cellar)' ran 47.16 ± 5.01 times faster than 'brew list --formula'

@razvanazamfirei Is this a strategy that could be used in bash/zsh completions too? May want to split it out into a command like brew formulae if so.

I'll have to check if it leads to any performance improvements since zsh caches some completions.

@razvanazamfirei Thanks! That could be a follow-up PR, doesn't need to block this one.

Library/Homebrew/completions/fish.erb

razvanazamfirei · 2023-04-10T15:09:54Z

Library/Homebrew/completions/fish.erb

+    command find (brew --prefix)/Library/Taps -mindepth 2 -maxdepth 2 -type d \
+    | string replace homebrew- "" \
+    | string replace (brew --prefix)/Library/Taps/ ""


❯ hyperfine 'brew tap' 'command find (brew --prefix)/Library/Taps -mindepth 2 -maxdepth 2 -type d | string replace homebrew- "" | string replace (brew --prefix)/Library/Taps/ ""' --shell fish --warmup 1 Benchmark 1: brew tap Time (mean ± σ): 455.4 ms ± 11.8 ms [User: 240.9 ms, System: 102.5 ms] Range (min … max): 441.5 ms … 474.4 ms 10 runs Benchmark 2: command find (brew --prefix)/Library/Taps -mindepth 2 -maxdepth 2 -type d | string replace homebrew- "" | string replace (brew --prefix)/Library/Taps/ "" Time (mean ± σ): 18.7 ms ± 1.2 ms [User: 6.1 ms, System: 8.3 ms] Range (min … max): 16.7 ms … 23.0 ms 94 runs Summary 'command find (brew --prefix)/Library/Taps -mindepth 2 -maxdepth 2 -type d | string replace homebrew- "" | string replace (brew --prefix)/Library/Taps/ ""' ran 24.40 ± 1.72 times faster than 'brew tap'

@razvanazamfirei Is this a strategy that could be used in bash/zsh completions too? May want to split it out into a command like brew formulae if so.

The path for the find command seems to be broken for me locally when I run brew tap c and try to autocomplete.

/u/l/Homebrew (fish-completions|✔) $ brew tap cfind: /usr/local/Library/Taps: No such file or directory find: /usr/local/Library/Taps: No such file or directory find: /usr/local/Library/Taps: No such file or directory find: /usr/local/Library/Taps: No such file or directory

I think maybe you should use brew --repo instead of brew --prefix here.

@apainintheneck, I made the change. Let me know if this fixes it and if you think I should add 2>/dev/null to the find command.

Yep, this fixed it. Thanks. I'm not sure if we need to redirect stderr to /dev/null right now. It shouldn't print to stderr at all right?

razvanazamfirei · 2023-04-10T15:11:08Z

Library/Homebrew/completions/fish.erb

@@ -104,22 +104,14 @@ function __fish_brew_ruby_parse_json -a file parser -d 'Parses given JSON file w
 end


This function becomes redundant if changes below are made.

Library/Homebrew/completions/fish.erb

MikeMcQuaid · 2023-04-11T08:17:46Z

Library/Homebrew/completions/fish.erb

 end

 function __fish_brew_suggest_formulae_installed
-    brew list --formula
+    command ls -1 (brew --cellar)


@razvanazamfirei Is this a strategy that could be used in bash/zsh completions too? May want to split it out into a command like brew formulae if so.

MikeMcQuaid · 2023-04-11T08:18:08Z

Library/Homebrew/completions/fish.erb

+    command find (brew --prefix)/Library/Taps -mindepth 2 -maxdepth 2 -type d \
+    | string replace homebrew- "" \
+    | string replace (brew --prefix)/Library/Taps/ ""


@razvanazamfirei Is this a strategy that could be used in bash/zsh completions too? May want to split it out into a command like brew formulae if so.

docs/Manpage.md

apainintheneck

Looking good so far. The installed formulas and casks complete much faster with these changes.

apainintheneck · 2023-04-12T01:43:07Z

Library/Homebrew/completions/fish.erb

+    command find (brew --prefix)/Library/Taps -mindepth 2 -maxdepth 2 -type d \
+    | string replace homebrew- "" \
+    | string replace (brew --prefix)/Library/Taps/ ""


The path for the find command seems to be broken for me locally when I run brew tap c and try to autocomplete.

/u/l/Homebrew (fish-completions|✔) $ brew tap cfind: /usr/local/Library/Taps: No such file or directory find: /usr/local/Library/Taps: No such file or directory find: /usr/local/Library/Taps: No such file or directory find: /usr/local/Library/Taps: No such file or directory

apainintheneck · 2023-04-12T01:52:21Z

Library/Homebrew/completions/fish.erb

+    command find (brew --prefix)/Library/Taps -mindepth 2 -maxdepth 2 -type d \
+    | string replace homebrew- "" \
+    | string replace (brew --prefix)/Library/Taps/ ""


I think maybe you should use brew --repo instead of brew --prefix here.

apainintheneck · 2023-04-12T02:10:26Z

Library/Homebrew/completions/fish.erb

-    # store the brew cache path in a var (because calling (brew --cache) is slow)
-    set -q __brew_cache_path
-    or set -gx __brew_cache_path (brew --cache)
-
-    if test -f "$__brew_cache_path/descriptions.json"
-        __fish_brew_ruby_parse_json "$__brew_cache_path/descriptions.json" \
-            '.each{ |k, v| puts([k, v].reject(&:nil?).join("\t")) }'
-    else
-        brew formulae
-    end
+    brew formulae


The results are different after this change because we no longer get any description related info.

Before

After

Not sure how important the descriptions are for most people though.

I think the descriptions came from $__brew_cache_path/descriptions.json. I don't have that file locally and haven't been able to figure out where it gets generated from.

Yeh, that file isn't generated by default. IMO the new behaviour makes more sense and the performance benefits are worth it.

MikeMcQuaid

Great work @razvanazamfirei!

MikeMcQuaid · 2023-04-12T11:24:41Z

Library/Homebrew/completions/fish.erb

-    # store the brew cache path in a var (because calling (brew --cache) is slow)
-    set -q __brew_cache_path
-    or set -gx __brew_cache_path (brew --cache)
-
-    if test -f "$__brew_cache_path/descriptions.json"
-        __fish_brew_ruby_parse_json "$__brew_cache_path/descriptions.json" \
-            '.each{ |k, v| puts([k, v].reject(&:nil?).join("\t")) }'
-    else
-        brew formulae
-    end
+    brew formulae


Yeh, that file isn't generated by default. IMO the new behaviour makes more sense and the performance benefits are worth it.

MikeMcQuaid · 2023-04-12T11:24:56Z

Library/Homebrew/completions/fish.erb

 end

 function __fish_brew_suggest_formulae_installed
-    brew list --formula
+    command ls -1 (brew --cellar)


@razvanazamfirei Thanks! That could be a follow-up PR, doesn't need to block this one.

MikeMcQuaid · 2023-04-12T11:25:35Z

Library/Homebrew/completions/fish.erb

@@ -143,7 +130,7 @@ function __fish_brew_suggest_casks_all -d "Lists locally available casks"
 end

 function __fish_brew_suggest_casks_installed -d "Lists installed casks"
-    brew list --cask -1
+    command ls -1 (brew --caskroom)


Same deal here: in a future PR this would be nice to make a command implemented in Bash I think if it provides speedups to either Bash and/or ZSH.

MikeMcQuaid · 2023-04-12T11:25:38Z

Library/Homebrew/completions/fish.erb

@@ -153,7 +140,9 @@ function __fish_brew_suggest_casks_outdated -d "Lists outdated casks with the in
 end

 function __fish_brew_suggest_taps_installed -d "List all available taps"
-    brew tap
+    command find (brew --repo)/Library/Taps -mindepth 2 -maxdepth 2 -type d \


Same deal here: in a future PR this would be nice to make a command implemented in Bash I think if it provides speedups to either Bash and/or ZSH.

razvanazamfirei force-pushed the fish-completions branch from 3a0716b to 0c22938 Compare April 8, 2023 17:19

razvanazamfirei marked this pull request as ready for review April 8, 2023 23:34

apainintheneck reviewed Apr 9, 2023

View reviewed changes

MikeMcQuaid reviewed Apr 10, 2023

View reviewed changes

razvanazamfirei commented Apr 10, 2023

View reviewed changes

MikeMcQuaid reviewed Apr 11, 2023

View reviewed changes

apainintheneck reviewed Apr 12, 2023

View reviewed changes

change some fish completions to brew --repo

5080010

razvanazamfirei force-pushed the fish-completions branch from 798a85d to 5080010 Compare April 12, 2023 09:15

MikeMcQuaid approved these changes Apr 12, 2023

View reviewed changes

MikeMcQuaid enabled auto-merge April 12, 2023 11:26

MikeMcQuaid merged commit 92d68e3 into Homebrew:master Apr 12, 2023
23 checks passed

razvanazamfirei deleted the fish-completions branch April 12, 2023 15:35

dawidd6 mentioned this pull request Apr 19, 2023

completions/fish: fix cask references for Linux #15272

Merged

7 tasks

github-actions bot added the outdated PR was locked due to age label May 14, 2023

github-actions bot locked as resolved and limited conversation to collaborators May 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update fish completions #15174

Update fish completions #15174

razvanazamfirei commented Apr 8, 2023 •

edited

apainintheneck left a comment

apainintheneck Apr 9, 2023

MikeMcQuaid Apr 10, 2023

razvanazamfirei Apr 10, 2023

MikeMcQuaid left a comment

MikeMcQuaid Apr 10, 2023

razvanazamfirei Apr 10, 2023

MikeMcQuaid Apr 11, 2023

razvanazamfirei Apr 12, 2023

MikeMcQuaid Apr 12, 2023

razvanazamfirei Apr 10, 2023

MikeMcQuaid Apr 11, 2023

apainintheneck Apr 12, 2023

apainintheneck Apr 12, 2023

razvanazamfirei Apr 12, 2023

apainintheneck Apr 13, 2023

razvanazamfirei Apr 10, 2023

MikeMcQuaid Apr 11, 2023

MikeMcQuaid Apr 11, 2023

apainintheneck left a comment

apainintheneck Apr 12, 2023

apainintheneck Apr 12, 2023

apainintheneck Apr 12, 2023

razvanazamfirei Apr 12, 2023

MikeMcQuaid Apr 12, 2023

MikeMcQuaid left a comment

MikeMcQuaid Apr 12, 2023

MikeMcQuaid Apr 12, 2023

MikeMcQuaid Apr 12, 2023

MikeMcQuaid Apr 12, 2023

	__brew_complete_installed_formulae() {
	local cur="${COMP_WORDS[COMP_CWORD]}"
	local installed_formulae
	installed_formulae="$(command ls "${HOMEBREW_CELLAR:-$(brew --cellar)}" 2>/dev/null)"
	while read -r line; do COMPREPLY+=("${line}"); done < <(compgen -W "${installed_formulae}" -- "${cur}")
	}

	__brew_complete_installed_casks() {
	local cur="${COMP_WORDS[COMP_CWORD]}"
	local installed_casks
	installed_casks="$(command ls "$(brew --caskroom)" 2>/dev/null)"
	while read -r line; do COMPREPLY+=("${line}"); done < <(compgen -W "${installed_casks}" -- "${cur}")
	}

		@@ -104,22 +104,14 @@ function __fish_brew_ruby_parse_json -a file parser -d 'Parses given JSON file w
		end

Update fish completions #15174

Update fish completions #15174

Conversation

razvanazamfirei commented Apr 8, 2023 • edited

apainintheneck left a 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

MikeMcQuaid left a 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

apainintheneck left a 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

Before

After

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid left a 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

razvanazamfirei commented Apr 8, 2023 •

edited