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

systemctl completions: add cat & edit commands #3757

Merged
merged 3 commits into from Feb 2, 2017

Conversation

Projects
None yet
4 participants
@nim65s
Contributor

nim65s commented Jan 22, 2017

This just adds autocompletions for "new" subcommands systemctl cat & systemctl edit.

@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Jan 22, 2017

Member

Do we need to add version detection for systemctl to make sure fish doesn't suggest options that aren't available?

Member

zanchey commented Jan 22, 2017

Do we need to add version detection for systemctl to make sure fish doesn't suggest options that aren't available?

@nim65s

This comment has been minimized.

Show comment
Hide comment
@nim65s

nim65s Jan 24, 2017

Contributor

edit appeared in 218 (https://github.com/systemd/systemd/blob/master/NEWS#L2445)
cat appeared in 209 (https://github.com/systemd/systemd/blob/master/NEWS#L4155)
systemd --version gives systemd 232 (and another line with compilation options)
So I guess it is not hard to add this check.

But I don't know if we "need" to add this check, and I don't know which distributions are using which versions of systemd… So I guess it's up to you. If you want this feature, I can include it in this PR :)

Contributor

nim65s commented Jan 24, 2017

edit appeared in 218 (https://github.com/systemd/systemd/blob/master/NEWS#L2445)
cat appeared in 209 (https://github.com/systemd/systemd/blob/master/NEWS#L4155)
systemd --version gives systemd 232 (and another line with compilation options)
So I guess it is not hard to add this check.

But I don't know if we "need" to add this check, and I don't know which distributions are using which versions of systemd… So I guess it's up to you. If you want this feature, I can include it in this PR :)

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Jan 24, 2017

Member

But I don't know if we "need" to add this check, and I don't know which distributions are using which versions of systemd… So I guess it's up to you. If you want this feature, I can include it in this PR :)

Yes, please add the check. Though please use systemctl, not systemd, since that's not commonly in $PATH.

While you are at it, there are a couple of options at the bottom with a comment that states they are new since 220. They should be version-guarded as well.

Member

faho commented Jan 24, 2017

But I don't know if we "need" to add this check, and I don't know which distributions are using which versions of systemd… So I guess it's up to you. If you want this feature, I can include it in this PR :)

Yes, please add the check. Though please use systemctl, not systemd, since that's not commonly in $PATH.

While you are at it, there are a couple of options at the bottom with a comment that states they are new since 220. They should be version-guarded as well.

@faho faho added this to the next-minor milestone Jan 24, 2017

@nim65s

This comment has been minimized.

Show comment
Hide comment
@nim65s

nim65s Jan 25, 2017

Contributor

Done :)
What do you think ?

Contributor

nim65s commented Jan 25, 2017

Done :)
What do you think ?

Show outdated Hide outdated share/completions/systemctl.fish
Show outdated Hide outdated share/completions/systemctl.fish
@nim65s

This comment has been minimized.

Show comment
Hide comment
@nim65s

nim65s Jan 25, 2017

Contributor

Thanks, TIL string function & english 🎉

Contributor

nim65s commented Jan 25, 2017

Thanks, TIL string function & english 🎉

@faho

faho approved these changes Jan 26, 2017

@faho faho merged commit b7cdb6d into fish-shell:master Feb 2, 2017

1 check failed

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

This comment has been minimized.

Show comment
Hide comment
@faho

faho Feb 2, 2017

Member

Thanks, merged!

Member

faho commented Feb 2, 2017

Thanks, merged!

@@ -1,8 +1,15 @@
set -l systemd_version (systemctl --version | string match "systemd*" | string replace -r "\D*(\d+)" '$1')

This comment has been minimized.

@floam

floam Feb 3, 2017

Member

If there is no systemctl command, you're going to get a lot of errors. This will go away when we stop loading completion scripts for non-existing commands.

/usr/local/Cellar/fish/HEAD-47ad707/share/fish/completions/systemctl.fish (line 1): systemctl --version | string match "systemd*" | string replace -r "\D*(\d+)"  '$1'
                                                                                    ^
in command substitution
	called on line -1 of file /usr/local/Cellar/fish/HEAD-47ad707/share/fish/completions/systemctl.fish

from sourcing file /usr/local/Cellar/fish/HEAD-47ad707/share/fish/completions/systemctl.fish
	called on standard input

in command substitution
	called on standard input

test: Missing argument at index 2
test: Missing argument at index 2
test: Missing argument at index 2
@floam

floam Feb 3, 2017

Member

If there is no systemctl command, you're going to get a lot of errors. This will go away when we stop loading completion scripts for non-existing commands.

/usr/local/Cellar/fish/HEAD-47ad707/share/fish/completions/systemctl.fish (line 1): systemctl --version | string match "systemd*" | string replace -r "\D*(\d+)"  '$1'
                                                                                    ^
in command substitution
	called on line -1 of file /usr/local/Cellar/fish/HEAD-47ad707/share/fish/completions/systemctl.fish

from sourcing file /usr/local/Cellar/fish/HEAD-47ad707/share/fish/completions/systemctl.fish
	called on standard input

in command substitution
	called on standard input

test: Missing argument at index 2
test: Missing argument at index 2
test: Missing argument at index 2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment