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 completions for connmanctl #3419

Merged
merged 5 commits into from Oct 16, 2016

Conversation

Projects
None yet
4 participants
@occivink
Contributor

occivink commented Oct 2, 2016

Completions for connmanctl, a connman (network management daemon) CLI. [man page]

The description strings are one-to-one copies from the help.

I didn't keep move-before, move-after because I thought that their use was rather limited interactively, but I could add them.
The peer-related commands (peer_service, peers) completion are limited because their documentation is lacking, and I couldn't test them locally (because I don't even really know what they do).

I would have really liked to have the name of the network as documentation when autocompleting a service, but it seems like the documentation strings cannot be dynamic, is that correct?
I also would have liked for enable and disable to only propose networks that are currently disabled or enabled respectively, but it was too many lines of script for the (limited) benefits.

@occivink

This comment has been minimized.

Show comment
Hide comment
@occivink

occivink Oct 2, 2016

Contributor

Looks like I could have declared the functions directly in the completion files, which seems preferable. Should I switch it?

Contributor

occivink commented Oct 2, 2016

Looks like I could have declared the functions directly in the completion files, which seems preferable. Should I switch it?

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Oct 2, 2016

Member

I'd suggest doing so.

Member

floam commented Oct 2, 2016

I'd suggest doing so.

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Oct 4, 2016

Member

I would have really liked to have the name of the network as documentation when autocompleting a service, but it seems like the documentation strings cannot be dynamic, is that correct?

It can. The description is everything in the line after a tab character. So if you do complete -c somecommand -a 'banana\tfruit', it'll add the completion "banana" with the description "fruit". This will also work if you use a command substitution, like complete -c somecommand -a '(printf "%s\t%s\n" banana fruit)'.

Member

faho commented Oct 4, 2016

I would have really liked to have the name of the network as documentation when autocompleting a service, but it seems like the documentation strings cannot be dynamic, is that correct?

It can. The description is everything in the line after a tab character. So if you do complete -c somecommand -a 'banana\tfruit', it'll add the completion "banana" with the description "fruit". This will also work if you use a command substitution, like complete -c somecommand -a '(printf "%s\t%s\n" banana fruit)'.

@faho faho added this to the fish 2.4.0 milestone Oct 4, 2016

Show outdated Hide outdated share/completions/connmanctl.fish
connmanctl vpnconnections | string replace -r '.* (.*)' '$1'
end
complete -f -c connmanctl -n "test (count (commandline -opc)) -lt 2" -a "state" -d "Shows if the system is online or offline"

This comment has been minimized.

@faho

faho Oct 4, 2016

Member

Usually you'll want a function that prints the currently used command.

In the simple case this can be __fish_seen_subcommand_from $commands, so you'd do

complete -f -c connmanctl -n "not __fish_seen_subcommand_from state technologies clock [...]".

This is to support options before commands, which is frequently allowed.

@faho

faho Oct 4, 2016

Member

Usually you'll want a function that prints the currently used command.

In the simple case this can be __fish_seen_subcommand_from $commands, so you'd do

complete -f -c connmanctl -n "not __fish_seen_subcommand_from state technologies clock [...]".

This is to support options before commands, which is frequently allowed.

This comment has been minimized.

@occivink

occivink Oct 4, 2016

Contributor

Actually connmanctl doesn't support options before commands (which I think is a bit weird anyway), should I still change them?

@occivink

occivink Oct 4, 2016

Contributor

Actually connmanctl doesn't support options before commands (which I think is a bit weird anyway), should I still change them?

This comment has been minimized.

@faho

faho Oct 5, 2016

Member

In that case, the simple thing is okay.

It might be nice to add a comment about it, though.

@faho

faho Oct 5, 2016

Member

In that case, the simple thing is okay.

It might be nice to add a comment about it, though.

complete -f -c connmanctl -n "test (count (commandline -opc)) -lt 2" -a "vpnconnections" -d "Display VPN connections"
complete -f -c connmanctl -n "test (count (commandline -opc)) -eq 2; and contains -- (commandline -opc)[2] vpnconnections" -a "(__fish_print_connman_vpnconnections)"
complete -f -c connmanctl -n "test (count (commandline -opc)) -lt 2" -a "session" -d "Enable or disable a session"
complete -f -c connmanctl -n "test (count (commandline -opc)) -eq 2; and contains -- (commandline -opc)[2] session" -a "on off connect disconnect config"

This comment has been minimized.

@faho

faho Oct 4, 2016

Member

Similarly, this would just be __fish_seen_subcommand_from session.

@faho

faho Oct 4, 2016

Member

Similarly, this would just be __fish_seen_subcommand_from session.

Show outdated Hide outdated share/completions/connmanctl.fish
@@ -0,0 +1,40 @@
function __fish_print_connman_services
connmanctl services | string replace -r '.* (.*)' '$1'

This comment has been minimized.

@faho

faho Oct 4, 2016

Member

If this is where you'd like the description to be generated, you could do string replace -r '(.*) (.*)' '$2\t$1'.

@faho

faho Oct 4, 2016

Member

If this is where you'd like the description to be generated, you could do string replace -r '(.*) (.*)' '$2\t$1'.

@occivink

This comment has been minimized.

Show comment
Hide comment
@occivink

occivink Oct 4, 2016

Contributor

Ah, good to know regarding the docs, I was looking at the -d switch and couldn't make it work dynamically.

Contributor

occivink commented Oct 4, 2016

Ah, good to know regarding the docs, I was looking at the -d switch and couldn't make it work dynamically.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Oct 5, 2016

Contributor

Removing the 2.4.0 milestone since there is no reason for this to be a blocking issue for that release and it is unclear when it will be merged.

Contributor

krader1961 commented Oct 5, 2016

Removing the 2.4.0 milestone since there is no reason for this to be a blocking issue for that release and it is unclear when it will be merged.

@krader1961 krader1961 modified the milestones: fish-future, fish 2.4.0 Oct 5, 2016

@faho

faho approved these changes Oct 5, 2016

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Oct 5, 2016

Member

Good to merge?

Member

floam commented Oct 5, 2016

Good to merge?

@occivink

This comment has been minimized.

Show comment
Hide comment
@occivink

occivink Oct 5, 2016

Contributor

Wait a bit, I noticed something that needs to be fixed

Contributor

occivink commented Oct 5, 2016

Wait a bit, I noticed something that needs to be fixed

@occivink

This comment has been minimized.

Show comment
Hide comment
@occivink

occivink Oct 5, 2016

Contributor

Okay after checking the implementation of the client I'm relatively confident in the regex for the services and vpnconnections. I'm not too familiar with the github merge process, is it possible to squash the commits during merging or should I do that on my end?

Contributor

occivink commented Oct 5, 2016

Okay after checking the implementation of the client I'm relatively confident in the regex for the services and vpnconnections. I'm not too familiar with the github merge process, is it possible to squash the commits during merging or should I do that on my end?

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Oct 5, 2016

Member

I'm not too familiar with the github merge process, is it possible to squash the commits during merging or should I do that on my end?

We'll do that. (Github offers a "Squash and merge", a "rebase and merge" and a default "merge", for more complicated things, you just git fetch and do it locally - e.g. with git rebase --interactive)

Member

faho commented Oct 5, 2016

I'm not too familiar with the github merge process, is it possible to squash the commits during merging or should I do that on my end?

We'll do that. (Github offers a "Squash and merge", a "rebase and merge" and a default "merge", for more complicated things, you just git fetch and do it locally - e.g. with git rebase --interactive)

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Oct 16, 2016

Member

@faho are you happy with this?

Member

floam commented Oct 16, 2016

@faho are you happy with this?

@faho faho merged commit 36352c0 into fish-shell:master Oct 16, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Oct 16, 2016

Member

Yes, merged. Sorry for the delay!

Member

faho commented Oct 16, 2016

Yes, merged. Sorry for the delay!

@faho faho modified the milestones: fish 2.4.0, fish-future Oct 16, 2016

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