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

[WIP] Add fish completions (Fixing #118) #151

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

harmtemolder
Copy link

@harmtemolder harmtemolder commented Dec 13, 2021

I dove into the way fish completions work and came up with a solution to add them for pass otp. They work, but still a couple of TODOs before it's ready for production:

  • Do not suggest a second subcommand after a first one is used
  • Do not suggest --clip on insert, append or validate
  • Do not hard-code the path to the main pass completions
  • Make sure the Makefile does not overwrite the main pass completions
  • Make sure the default FISHCOMPDIR is in $fish_complete_path, and before the main pass completions' location

@harmtemolder harmtemolder mentioned this pull request Dec 13, 2021
@harmtemolder harmtemolder changed the title Add fish completions (Fixing #118) [WIP] Add fish completions (Fixing #118) Dec 14, 2021
@@ -20,6 +21,8 @@ install:
install -m0755 $(PROG).bash "$(DESTDIR)$(SYSTEM_EXTENSION_DIR)/$(PROG).bash"
install -d "$(DESTDIR)$(BASHCOMPDIR)/"
install -m 644 pass-otp.bash.completion "$(DESTDIR)$(BASHCOMPDIR)/pass-otp"
install -d "$(DESTDIR)$(FISHCOMPDIR)/"
install -m 644 pass.fish "$(DESTDIR)$(FISHCOMPDIR)/pass.fish"

Choose a reason for hiding this comment

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

I think it should be called pass-otp since it might interfere with pass?

Copy link
Author

Choose a reason for hiding this comment

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

I actually tried that first, but it seemed like fish uses the filename to apply its completions to a specific command (in this case pass). If you know of a way to name this file pass-otp.fish I'm all for it

Choose a reason for hiding this comment

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

But won't that overwrite the default pass.fish?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, that's why by default this pass.fish installs into /etc/fish/completions and reads the default pass.fish from /usr/share/fish/vendor_completions.d/pass.fish. That is where the default ones are on my system, but I can't guarantee that on other systems. That's why checkboxes 3 and 4 are still open

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, the setup for completions is kind of messy and I haven't looked at this for a while. Lemme check upstream and get back to you.

Copy link
Owner

Choose a reason for hiding this comment

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

So, the ideal solution would be for pass to delegate to extension completion scripts itself, similar to what it does for pass git and pass grep here: https://git.zx2c4.com/password-store/commit/?id=06f499994071bb6131244218b25d637103afe1d5. This should be a reasonable change to submit upstream.

@apastuszak
Copy link

If you installed pass on a Mac using homebrew the path to the completion file is:

/opt/homebrew/share/fish/vendor_completions.d/pass.fish

Shouldn't the path be set by the package maintainer for whatever distro this is packaged for?

This is working great on my Mac.

@harmtemolder
Copy link
Author

This is working great on my Mac.

Do you mean these added completions?

Shouldn't the path be set by the package maintainer for whatever distro this is packaged for?

I guess it should, but I'm not in the trade of making packages and don't know how to do this. That means I'd need help to finish this PR.

@tadfisher Can you—or do you know anyone who can—help me figure out a way to dynamically get the location of the original pass's completions so that I don't have to hard-code them?

@vonpupp
Copy link

vonpupp commented Sep 10, 2023

I made my own completion following @tadfisher's comments. These are on a separate file named 'pass-otp.fish' and instead of copying it to /usr/share/fish/vendor_completions.d/pass.fish, I copy to my private XDG config file (.config/fish/completions/), so it doesn't conflict with pass.fish. I know nothing about fish completions, I basically copied and pasted and followed along the pass git command as suggested, but it seems to be working fine.

Personally I manage that file within my dotfiles, but if you want I can open a PR to contribute this file. I am not sure how to add that to the Makefile but it shouldn't be that hard.

@BarbzYHOOL
Copy link

I made my own completion following @tadfisher's comments. These are on a separate file named 'pass-otp.fish' and instead of copying it to /usr/share/fish/vendor_completions.d/pass.fish, I copy to my private XDG config file (.config/fish/completions/), so it doesn't conflict with pass.fish. I know nothing about fish completions, I basically copied and pasted and followed along the pass git command as suggested, but it seems to be working fine.

Personally I manage that file within my dotfiles, but if you want I can open a PR to contribute this file. I am not sure how to add that to the Makefile but it shouldn't be that hard.

i tried this but it doesn't help one bit with this issue at least #187

zvyn added a commit to zvyn/pass-otp that referenced this pull request Jan 6, 2024
This is a subset of tadfisher#151, only implementing the most relevant completion
(see tadfisher#118), i.e. completing password names.
zvyn added a commit to zvyn/pass-otp that referenced this pull request Jan 6, 2024
This is a subset of tadfisher#151, only implementing the most relevant completion
(see tadfisher#118), i.e. completing password names.
@pickfire
Copy link

pickfire commented Jun 29, 2024

I guess this should be closed since it's superseeded by another PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants