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 repo completion for zypper #4325

Merged
merged 4 commits into from Sep 6, 2017
Merged

Add repo completion for zypper #4325

merged 4 commits into from Sep 6, 2017

Conversation

Sam0523
Copy link
Contributor

@Sam0523 Sam0523 commented Aug 13, 2017

No description provided.

@@ -1,7 +1,8 @@
# completion for zypper

set -g __fish_zypper_all_commands shell sh repos lr addrepo ar removerepo rr renamerepo nr modifyrepo mr refresh ref clean services ls addservice as modifyservice ms removeservice rs refresh-services refs install in remove rm verify ve source-install si install-new-recommends inr update up list-updates lu patch list-patches lp dist-upgrade dup patch-check pchk search se info if patch-info pattern-info product-info patches pch packages pa patterns pt products pd what-provides wp addlock al removelock rl locks ll cleanlocks cl versioncmp vcmp targetos tos licenses source-download
set -g __fish_zypper_all_commands shell sh repos lr addrepo ar removerepo rr renamerepo nr modifyrepo mr refresh ref clean cc services ls addservice as modifyservice ms removeservice rs refresh-services refs install in remove rm verify ve source-install si install-new-recommends inr update up list-updates lu patch list-patches lp dist-upgrade dup patch-check pchk search se info if patch-info pattern-info product-info patches pch packages pa patterns pt products pd what-provides wp addlock al removelock rl locks ll cleanlocks cl versioncmp vcmp targetos tos licenses source-download
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, It is better to append new values to lists of this nature. If for no other reason than to make figuring out what changed easier.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah! I see why you did it this way. It mimics the other commands which pair the short and long subcommands. Ignore my previous comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly.

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.

  • Move function into the completion script

  • Ideally port to string.

@@ -0,0 +1,3 @@
function __fish_print_zypp_repos -d "Print repositories of zypper package manager"
Copy link
Member

Choose a reason for hiding this comment

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

Since this is not used anywhere else, can you just stick it in the completion script?

Also I'd appreciate it if this used our string builtin instead.

Something like

for file in /etc/zypp/repos.d/*.repo
    string replace -rf '\[(.*)\]' '$1' <$file
end

(This will match something like [bla] and replace it with bla. The "-f" for "--filter" will only use lines where a replacement can be performed)

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 noticed that pacman has a standalone function __fish_print_pacman_repos, so I thought I should put that into another file... Is it a good idea to write a function __fish_print_repos and do the distribution detection inside it? Just like __fish_print_packages?

Also thanks for such detailed explanation! Will replace sed with string soon.

Copy link
Member

Choose a reason for hiding this comment

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

I noticed that pacman has a standalone function __fish_print_pacman_repos, so I thought I should put that into another file...

That's also used in other completions (pacman has a bit of a culture of things wrapping it). This one isn't.

Is it a good idea to write a function __fish_print_repos and do the distribution detection inside it? Just like __fish_print_packages?

Please no. I don't like that one as it is, because any completion that uses it wants exactly one of the options. That means the detection is actually unnecessary, and can even be wrong (e.g. if you have apt-cache installed and try to use pacman, you'll get the wrong packages).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I get it.

@Sam0523
Copy link
Contributor Author

Sam0523 commented Aug 15, 2017

@faho Changes done. Please review.

@faho faho merged commit 81becc5 into fish-shell:master Sep 6, 2017
@faho
Copy link
Member

faho commented Sep 6, 2017

Merged, thanks, and sorry for the delay!

zanchey pushed a commit that referenced this pull request Sep 6, 2017
* Add repo completion for zypper

* Replace sed with string in __fish_print_zypp_repos

* Move function into completion script

* Update zypper completion

add subcommand packages to __fish_zypper_repo_commands

(cherry picked from commit 81becc5)
@zanchey zanchey modified the milestones: fish 2.7.0, fish-3.0 Sep 6, 2017
@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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants