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

feat(completions): add sops completions #8821

Merged
merged 9 commits into from
Apr 13, 2022
Merged

feat(completions): add sops completions #8821

merged 9 commits into from
Apr 13, 2022

Conversation

budimanjojo
Copy link
Contributor

Description

Add SoPS shell completion.

Issue I can't solve

  • I don't know how to check if previous primary flag is a flag that require value. For example, when I want to type in: sops --add-age "value" --decrypt If I type in sops --add-age and put in the value that it needs, then I type in --, it should give me list of flags that I can put in after that, but it shows me nothing but global flag. The workaround for user is to type it in like this sops --add-age="value" --decrypt

TODOs:

  • I still need help in the issue above

share/completions/sops.fish Outdated Show resolved Hide resolved
share/completions/sops.fish Outdated Show resolved Hide resolved
share/completions/sops.fish Outdated Show resolved Hide resolved
share/completions/sops.fish Outdated Show resolved Hide resolved
share/completions/sops.fish Outdated Show resolved Hide resolved
share/completions/sops.fish Outdated Show resolved Hide resolved
end
set -l cmd (commandline -cp)
if set -q argv[2]
if string match -qr -- "sops\s+$argv[1]\s+$argv[2]\s+\S*" $cmd
Copy link
Member

Choose a reason for hiding this comment

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

Can you try to replace uses of this function with __fish_seen_subcommand_from?

I believe if there are multiple levels of subcommands, it's okay to use __fish_seen_subcommand_from subcmd && __fish_seen_subcommand_from subsubcmd (not always fully correct but good enough)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean deleting this whole __fish_sops_commands function and use __fish_seen_subcommand_from instead?

Copy link
Member

Choose a reason for hiding this comment

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

Do you mean deleting this whole __fish_sops_commands function and use __fish_seen_subcommand_from instead?

yep

Copy link
Member

Choose a reason for hiding this comment

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

even better, replace it with a function that uses argparse to decide which subcommand is active (similar to __fish_sudo_print_remaining_args)

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 reason why I didn't use __fish_seen_subcommand_from subcmd1 && __fish_seen_subcommand_from subcmd2 for multiple levels subcommands is because it accepts anything as long as there are the word subcmd1 and subcmd2 ignoring the placement. For example, if I want group as subcmd1 and add as subcmd2 then even if I write it as sops blablabla add groups it will still return true. While the function that I'm using it right now (I took the code from __btrfs_commands and __btrfs_command_groups) will return false unless the command is sops groups add and nothing else.

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 might go with the even better "similar to __fish_sudo_print_remaining_args" approach, but it might need some time because I'm still figuring out how does argparse work. T.T

Copy link
Member

Choose a reason for hiding this comment

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

Ok makes sense. I think copying __fish_sudo_print_remaining_args is the way to go. We should probably add more examples to the argparse docs.

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 have modified the way it works, please have another look @krobelus. I don't know if the code is good as I'm not experienced enough for this (not even sure what I did lol), please correct my code if needed. :)

@budimanjojo
Copy link
Contributor Author

budimanjojo commented Apr 10, 2022

@krobelus Thanks for the reviews! I have commited some of your suggestions, and commented on those I don't quite understand. Please have another look :)
I don't know why the check is not successful though, completion files shouldn't affect builds right?

@krobelus
Copy link
Member

I don't know why the check is not successful though, completion files shouldn't affect builds right?

yeah, no need to worry. We have some flaky tests.

@krobelus krobelus merged commit b475878 into fish-shell:master Apr 13, 2022
@krobelus krobelus added this to the fish 3.5.0 milestone Apr 13, 2022
@krobelus
Copy link
Member

Thanks!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants