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

gpg.fish refactor #6139

Closed
wants to merge 3 commits into from
Closed

gpg.fish refactor #6139

wants to merge 3 commits into from

Conversation

LawAbidingCactus
Copy link
Contributor

Description

Refactors gpg.fish to conditionally activate completions based on gpg version, as discussed in #6062:

The following was discussed briefly in #2401, but remained unresolved. On many systems, gpg is symlinked to gpg2-- when this is the case, it would be more appropriate to use these completions than fish's current ones (with gpg2 in the completion script replaced with gpg). gpg.fish could test for the GPG version and activate version-specific completions depending on the result.

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.md

@LawAbidingCactus LawAbidingCactus changed the title Gpg fish refactor gpg.fish refactor Sep 24, 2019
@LawAbidingCactus LawAbidingCactus changed the title gpg.fish refactor gpg.fish refactor Sep 24, 2019
Copy link
Member

@zanchey zanchey left a comment

Choose a reason for hiding this comment

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

As a general overview, I don't think this is the right approach - there is lots of duplicated code which I think could be simplified.

I wonder about:

  • breaking gpg2 completions out into __fish_complete_gpg2, which takes a __fish_complete_gpg2_command argument, and which uses $__fish_complete_gpg2_command instead of gpg2 everywhere (in functions and completion arguments
  • in gpg2.fish, call __fish_complete_gpg_command gpg2
  • in gpg.fish, check if gpg is 2.x, and if so call __fish_complete_gpg2 gpg; otherwise use the gpg1 completions

Would that work?

share/completions/gpg.fish Outdated Show resolved Hide resolved
share/completions/gpgbackup.fish Outdated Show resolved Hide resolved
ridiculousfish added a commit that referenced this pull request Oct 6, 2019
Revert "gut gpg.fish/gpg1.fish/gpg2.fish; migrate functionality to __fish_complete_gpg.fish"

This reverts commit d558218.

Revert "break version-specific completions out into independent function;"

This reverts commit 9160e77.

Revert "split gpg2- and gpg1-specific completions to conditional block"

This reverts commit a069b95.
@ridiculousfish
Copy link
Member

I merged this but then saw the comments, so reverted the merge.

@LawAbidingCactus
Copy link
Contributor Author

@ridiculousfish: Yeah, sorry-- I implemented @zanchey's idea for deduplicating redundant completion code and decided to see if I could take it a little further. It's not ready yet, since I haven't had time to work on it lately.

@LawAbidingCactus
Copy link
Contributor Author

LawAbidingCactus commented Oct 10, 2019 via email

@ridiculousfish
Copy link
Member

note zanchey is on vacation so this may sit a while. If you are keen to have this merged before their return let me know.

Copy link
Member

@zanchey zanchey left a comment

Choose a reason for hiding this comment

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

Looks great; one comment-related change but otherwise good to go.

share/functions/__fish_complete_gpg.fish Outdated Show resolved Hide resolved
@krobelus
Copy link
Member

Merged as 21a6a19, thanks for all the work!

@krobelus krobelus closed this Oct 29, 2019
@zanchey
Copy link
Member

zanchey commented Oct 30, 2019

Thanks for merging @krobelus, I was hoping to get to this today.

@zanchey zanchey mentioned this pull request Oct 30, 2019
@krobelus krobelus added this to the fish 3.1.0 milestone Oct 30, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 16, 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