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 completion for port #4737

Closed
wants to merge 6 commits into from
Closed

Add completion for port #4737

wants to merge 6 commits into from

Conversation

@kfkonrad
Copy link
Contributor

@kfkonrad kfkonrad commented Feb 19, 2018

Description

Adds completion for the MacPorts package manager for macOS. All subcommands and options listed in the port manfile have been added. The package list is generated with the adapted __fish_print_packages function. Because port is rather slow, I included cashing. The BSD commands for accessing file age are vastly different from the linux implementation, thus my approach to checking age differs from all others in __fish_print_packages.

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages. (none made)
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.md
end
end
# Remove trailing whitespace and pipe into cache file
port echo all | awk '{$1=$1};1' >$cache_file
Copy link
Member

@zanchey zanchey Feb 20, 2018

Choose a reason for hiding this comment

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

This will block until the cache file is filled, so it might be worth running this in the background. That means you don't get completions on the first use, but that's better than hanging waiting for a slow command - see what the other approaches taken here are.

Loading

Copy link
Member

@zanchey zanchey left a comment

I've made some further suggestions below.

Loading

return 0
end

function __fish_port_use_package --description 'Test if port command should have packages as potential completion'
Copy link
Member

@zanchey zanchey Feb 20, 2018

Choose a reason for hiding this comment

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

You can just use the __fish_seen_subcommand_from function here.

Loading

@@ -0,0 +1,115 @@
#completion for port

function __fish_port_no_subcommand --description 'Test if port has yet to be given the subcommand'
Copy link
Member

@zanchey zanchey Feb 20, 2018

Choose a reason for hiding this comment

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

You can use the __fish_use_subcommand function instead of this function.

Loading

complete -f -n '__fish_port_no_subcommand' -c port -a 'version' --description 'Print the MacPorts version'
complete -f -n '__fish_port_no_subcommand' -c port -a 'work' --description 'Displays the path to the work directory for portname'

complete -c apt-get -s v --description 'Verbose mode, generates verbose messages'
Copy link
Member

@zanchey zanchey Feb 20, 2018

Choose a reason for hiding this comment

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

Should these apt-get entries be here?

Loading

Copy link
Contributor Author

@kfkonrad kfkonrad Feb 20, 2018

Choose a reason for hiding this comment

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

The options and descriptions are correct, but the command should be port, not apt-get. I used apt-get completions as a template, seems to be a relic from that.

Loading


complete -c port -n '__fish_port_use_package' -a '(__fish_print_packages)' --description 'Package'

complete -f -n '__fish_port_no_subcommand' -c port -a 'activate' --description 'Set the status of an previously installed version of a port to active'
Copy link
Member

@zanchey zanchey Feb 20, 2018

Choose a reason for hiding this comment

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

I'm not sure if the description text here comes from the manual page, but it might be better to use shorter and more active voiced descriptions: eg "Activate a previously-installed port". This comment applies to many of the descriptions below - more than 50 or so characters will generally be truncated on smaller screens.

Loading

Copy link
Contributor Author

@kfkonrad kfkonrad Feb 20, 2018

Choose a reason for hiding this comment

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

Yes, those are taken from the respective manpages. I'll try and shorten the more verbose descriptions.

Loading

complete -f -n '__fish_port_no_subcommand' -c port -a 'rev-upgrade' --description 'Manually check for broken binaries and rebuild ports containing broken binaries'
complete -f -n '__fish_port_no_subcommand' -c port -a 'search' --description 'Search for a port using keywords'
complete -f -n '__fish_port_no_subcommand' -c port -a 'select' --description 'For a given group, selects a version to be the default by creating appropriate symbolic links'
complete -f -n '__fish_port_no_subcommand' -c port -a 'selfupdate' --description 'Upgrade MacPorts itself and update the port definition files.'
Copy link
Member

@zanchey zanchey Feb 20, 2018

Choose a reason for hiding this comment

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

No period (.) needed.

Loading

complete -f -n '__fish_port_no_subcommand' -c port -a 'test' --description 'Run test phase of a port'
complete -f -n '__fish_port_no_subcommand' -c port -a 'unarchive' --description 'Extract the destroot of the given ports from a binary archive'
complete -f -n '__fish_port_no_subcommand' -c port -a 'uninstall' --description 'Remove a previously installed port'
complete -f -n '__fish_port_no_subcommand' -c port -a 'unload' --description 'A shortcut to launchctl, like load, but unloads the daemon'
Copy link
Member

@zanchey zanchey Feb 20, 2018

Choose a reason for hiding this comment

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

This can just be "unload the daemon" - a similar change can be made to the load description.

Loading

end
# Remove trailing whitespace and pipe into cache file
printf "all\ncurrent\nactive\ninactive\ninstalled\nuninstalled\noutdated" >$cache_file
port echo all | awk '{$1=$1};1' >>$cache_file &
Copy link
Member

@zanchey zanchey Feb 20, 2018

Choose a reason for hiding this comment

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

Nice one.

Loading

@kfkonrad
Copy link
Contributor Author

@kfkonrad kfkonrad commented Feb 20, 2018

Ok, valid points. I changed all descriptions longer than 50 characters and made some descriptions clearer. My custom commands are gone and I use __fish_seen_subcommand_from and __fish_use_subcommand instead, thanks for letting me know those functions existed :)

Loading

@kfkonrad
Copy link
Contributor Author

@kfkonrad kfkonrad commented Feb 22, 2018

Is there anything else I should change before you can merge this request? The only change requested GitHub still lists is just you commenting 'nice one' on my change of __fish_print_packages

Loading

@zanchey
Copy link
Member

@zanchey zanchey commented Feb 24, 2018

No, that looks good to me!

Loading

@zanchey
Copy link
Member

@zanchey zanchey commented Feb 24, 2018

Merged as 99200d3..8536a68 - thanks!

Loading

@zanchey zanchey closed this Feb 24, 2018
@zanchey zanchey added this to the fish-3.0 milestone Feb 24, 2018
@zanchey zanchey self-assigned this Feb 24, 2018
@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.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants