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

Fix plugin completion results parsing for ShellCompDirectiveFilterFileExt #4136

Merged
merged 1 commit into from
Apr 4, 2023

Conversation

laurazard
Copy link
Member

@laurazard laurazard commented Mar 29, 2023

Compose V2 (and other plugins eventually) provide the extensions they want the shell to filter the autocompletion of files for:

$ /usr/libexec/docker/cli-plugins/docker-compose __completeNoDesc -f ""
yaml
yml
:8
Completion ended with directive: ShellCompDirectiveFilterFileExt

The current script does not process these results correctly, and instead we just provide a y (common beginning between yaml and yml)

$ docker compose -f <TAB><TAB>
yaml
yml

which is not the desired outcome.

- What I did

Check if the completion flag is :8 (ShellCompDirectiveFilterFileExt) and format a valid glob pattern to feed into _filedir when this is the case

- How to verify it

Load the new completion script, and type

docker compose -f <TAB><TAB>

verify that completions are provided for files with the extension .yaml or .yml,

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@codecov-commenter
Copy link

codecov-commenter commented Mar 29, 2023

Codecov Report

Merging #4136 (4896123) into master (88924b1) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4136   +/-   ##
=======================================
  Coverage   59.16%   59.16%           
=======================================
  Files         287      287           
  Lines       24716    24716           
=======================================
  Hits        14623    14623           
  Misses       9209     9209           
  Partials      884      884           

@thaJeztah
Copy link
Member

@tianon @albers ptal 🤗

contrib/completion/bash/docker Outdated Show resolved Hide resolved
local rawResult=$(eval "${resultArray[*]}" 2> /dev/null)
local result=$(grep -v '^:[0-9]*$' <<< "$rawResult")

# Compose V2 completions sometimes returns returns `:8` (ShellCompDirectiveFilterFileExt)
Copy link
Contributor

Choose a reason for hiding this comment

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

If anyone else reviewing this was unfamiliar with what this means, it's a Cobra thing, and https://github.com/spf13/cobra/blob/4dd4b25de38418174a6e859e8a32eaccca32dccc/completions_test.go#L1046-L1047 is the best reference I could find for it being :8 (there's probably a better one, but that's what my search brought me to 😅).

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I'd thought about adding some link explaining it better and then totally forgot :|

@thaJeztah
Copy link
Member

I'll skip merging this one for now, to have a look if we want to add Tianon's suggestions (as it's marked for cherry-picking, it makes it a bit cleaner to do the backport "in one go" instead of a PR-and-follow-up).

Copy link
Collaborator

@albers albers left a comment

Choose a reason for hiding this comment

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

After finally realizing that the Completion ended with directive: ShellCompDirectiveFilterFileExt line actually goes to stderr: LGTM, thanks @laurazard

@thaJeztah
Copy link
Member

Just checking up on this; Is the suggestion from @tianon (echo -> tail) something to consider, or "too minor to bother")? #4136 (comment)

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants