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 shell completion for --device and --reader #443

Merged
merged 8 commits into from
Jan 23, 2024
Merged

Conversation

emlun
Copy link
Member

@emlun emlun commented Jul 23, 2021

(Experimental/speculative)

Fixes #439.

In bash, the suggestions will simply be the available serial numbers. In more capable shells like zsh or fish, the suggestions will be annotated with the same device descriptions as shown by ykman list.

Example:
2021-07-23-20:47:57_1390x472

Marking this as a draft since this would bump the Click dependency to minimum version 8, and we might not want to require that. Version 7 also supports a variant of this, but without the "suggestion help" feature of the more capable shells.

In bash, the suggestions will simply be the available serial numbers. In more
capable shells like zsh or fish, the suggestions will be annotated with the same
device descriptions as shown by `ykman list`.
@dainnilsson
Copy link
Member

Looks interesting. I think we could fairly easily degrade on older versions of Click rather than require Click 8.

The thing that concerns me about this is that enumerating attached YubiKeys with serial numbers is a pretty slow/heavy operation, which also interrupts anything those YubiKeys are currently doing (eg. killing scdaemon, etc.). I'm a bit worried that this would be unexpected to users.

@emlun
Copy link
Member Author

emlun commented Aug 17, 2021

Yeah, good point. I experimented a bit more with an explicit opt-in step... try it out. 🙂 I haven't tried it in zsh or fish, but it works pretty well in bash at least. But in the end this might be too much complexity for too little gain.

@kurtmckee
Copy link
Contributor

Came to this repo in hopes that bash completion had been implemented, including commands, subcommands, and device numbers. Probably the Click 8 dependency isn't an important consideration 2 years on.

@emlun
Copy link
Member Author

emlun commented Nov 13, 2023

Right you are, the dependency in pyproject.toml on main now already specifies 8 as the minimum Click version. So perhaps it's time to revisit this @dainnilsson?

@dainnilsson
Copy link
Member

The main issue with this wasn't the Click 8 requirement, it was the operation requiring interrupting several attached devices and being slow. Unless that changes this needs to be locked away behind a secret flag, and I don't really see it as very useful in that case. I could probably be convinced otherwise though.

@emlun
Copy link
Member Author

emlun commented Nov 14, 2023

Yes, that is still the case, and the feature as implemented is disabled unless the environment variable _YKMAN_EXPERIMENTAL_COMPLETE_DEVICE exists. Sounds like at least @kurtmckee would still use it? I would too, for what it's worth.

@dainnilsson
Copy link
Member

After some internal discussion I've pushed a new commit with an alternative approach:

Any time devices are enumerated as part of standard usage (most ykman commands will do this) the list of devices is cached in an LRU cache, for use with tab-completion later on. This means that the devices that will complete aren't necessarily present at the moment, but have been "recently" present. The benefit of this is that we never interrupt devices specifically for tab completing purposes, and that it is much faster to actually invoke tab completion. One exception is when running a command without specifying a serial when multiple devices are attached: Here I added an explicit update to the cache as it is very likely the user will immediately try to re-run the command with a serial specified.

The limit on the cache is currently set to 3 devices, which is probably too low, to make testing the behavior a bit easier.

@dainnilsson dainnilsson marked this pull request as ready for review January 23, 2024 10:17
@dainnilsson dainnilsson merged commit ea95c22 into main Jan 23, 2024
12 checks passed
@dainnilsson dainnilsson deleted the complete-devices branch January 23, 2024 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

[Feature Request] Add autocomplete for --device and --reader parameters.
3 participants