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

Add eopkg completion support #4600

Merged
merged 7 commits into from Dec 21, 2017
Merged

Add eopkg completion support #4600

merged 7 commits into from Dec 21, 2017

Conversation

yursan9
Copy link
Contributor

@yursan9 yursan9 commented Dec 15, 2017

Add support for eopkg in __fish_print_packages function, and
add new completion eopkg.fish in share/completions

Add support for eopkg in __fish_print_packages function, and
add new completion eopkg.fish in share/completions
Copy link
Member

@faho faho left a comment

Choose a reason for hiding this comment

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

  • Simplification

  • Some nits regarding descriptions

Nice work so far!

# Eopkg additional completion test
function __fish_eopkg_package_ok -d 'Test if eopkg should have packages as completion candidate'
for i in (commandline -poc)
if contains -- $i upgrade up remove rm install it info check
Copy link
Member

Choose a reason for hiding this comment

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

This entire function can be simplified to complete -c eopkg -n '__fish_seen_subcommand_from upgrade up remove rm #...' -a '(__fish_print_packages)'.


function __fish_eopkg_component_ok -d 'Test if eopkg should have components as posible completion'
for i in (commandline -poc)
if contains -- $i -c --component
Copy link
Member

Choose a reason for hiding this comment

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

This can also be simplified - just use complete's option options:

complete -c eopkg -s c -l component -a '(__fish_eopkg_print_components)'


# Eopkg subcommand
function __fish_eopkg_subcommand
set subcommand $argv[1]
Copy link
Member

Choose a reason for hiding this comment

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

This can also be done via the -a/--argument-names option to function. I.e.

function __fish_eopkg_subcommand -a subcommand
     set -e argv[1] # or use `$argv[2..-1]` later
     complete -f #...

Copy link
Member

Choose a reason for hiding this comment

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

Note: Using $argv[$x] with $x != 1 is okay since 2.7.0 - before it would print an error if it $argv wasn't long enough.

complete -f -c eopkg -n '__fish_use_subcommand' -a $shortcut $argv
end

function __fish_eopkg_using_subcommand -d 'Test if given subcommand is used'
Copy link
Member

Choose a reason for hiding this comment

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

This is almost exactly __fish_seen_subcommand_from.


# Print additional completion
function __fish_eopkg_print_components -d "Print list of components"
eopkg list-components -N | cut -d ' ' -f 1
Copy link
Member

Choose a reason for hiding this comment

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

This cut can be replaced with string replace -r ' .*' '', removing one fork.

complete -f -c eopkg -n '__fish_eopkg_repo_ok' -a "(__fish_eopkg_print_repos)" -d "repository"

# Setup eopkg subcommand with shortcut
__fish_eopkg_subcommand_with_shortcut upgrade up -d "Upgrades eopkg packages"
Copy link
Member

Choose a reason for hiding this comment

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

Descriptions should be kept as short as possible - is the eopkg necessary here? That's the tool we're using, I wouldn't expect it to upgrade deb packages or rpms.


# Setup eopkg subcommand with shortcut
__fish_eopkg_subcommand_with_shortcut upgrade up -d "Upgrades eopkg packages"
__fish_eopkg_option_with_shortcut upgrade up -l security-only -d "Security related package upgrades only"
Copy link
Member

Choose a reason for hiding this comment

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

Cut the "package" - "Security related upgrades only".

# Setup eopkg subcommand with shortcut
__fish_eopkg_subcommand_with_shortcut upgrade up -d "Upgrades eopkg packages"
__fish_eopkg_option_with_shortcut upgrade up -l security-only -d "Security related package upgrades only"
__fish_eopkg_option_with_shortcut upgrade up -s n -l dry-run -d "Do not perform any action, just show what would be done"
Copy link
Member

Choose a reason for hiding this comment

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

Wow, we have a bunch of different descriptions for dry-run options - grep dry share/completions/*.fish.

I kinda like Show what would be done from the bzr completions, or Display actions without running from fossil.

__fish_eopkg_option_with_shortcut install it -s c -l component -f -d "Install component's and recursive components' packages"

__fish_eopkg_subcommand_with_shortcut remove rm -d "Remove eopkg packages"
__fish_eopkg_option_with_shortcut remove rm -l purge -d "Removes everything including changed config files of the package"
Copy link
Member

Choose a reason for hiding this comment

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

You're using the imperative mood elsewhere - "Remove ...". Do that here as well:

"Remove everything including changed config files"? Or "Remove changed config files as well"?


__fish_eopkg_subcommand_with_shortcut history hs -d "History of eopkg operations"
__fish_eopkg_option_with_shortcut history hs -s l -l last -x -d "Output only the last n operations"
__fish_eopkg_option_with_shortcut history hs -s t -l takeback -x -d "Takeback to the state after the given operation finished"
Copy link
Member

Choose a reason for hiding this comment

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

"finished" is kinda superfluous here, no? "Takeback to the state after the given operation"?

@faho faho added this to the fish-3.0 milestone Dec 16, 2017
@yursan9
Copy link
Contributor Author

yursan9 commented Dec 16, 2017

Thank you, @faho! I'll try to implement your review. Completion's description was pulled from eopkg help output, and my English writing skills is terrible.

@faho faho merged commit 94ff789 into fish-shell:master Dec 21, 2017
@faho
Copy link
Member

faho commented Dec 21, 2017

Merged, thanks!

@faho faho added the release notes Something that is or should be mentioned in the release notes label Dec 21, 2017
@yursan9 yursan9 deleted the eopkg-support branch December 22, 2017 09:55
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
completions enhancement release notes Something that is or should be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants