-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Improve host completion #3454
Merged
Merged
Improve host completion #3454
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
5163eff
[shell] don’t print error on non-existent SSH files
calestyo 2e989ed
[shell] move username prefixing code where needed
calestyo 44066bf
[shell] make `__fzf_list_hosts()` definable by the user
calestyo 93d02e2
[bash] try to use bash-completions’s _known_hosts_real() for getting …
calestyo 9a86b70
[bash] statically define __fzf_list_hosts() with either method
calestyo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine for now. But I'll have to reconsider this if they decide to change the way the function works in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they're consolidating their function naming right now, so me might need to adapt that soon to work with the new name and fall back to the old one... but that shouldn't be too hard.
Other than that... I'd rather not expect any deeper changes to what the function does, do you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that's a deal breaker for me. I don't want to ship a product that might not work because of a sudden change in an implicit dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with their code, and I just assumed _known_hosts_real was one of their documented public API that is guaranteed to be backward-compatible. If that's not the case, I'm not going to make fzf depend on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, bash-completion is not the C standard or so... I think it's reasonable to expect that they have some level of stability in their ABI (since people use it), but nothing is ever set fully in stone… not even POSIX and the likes.
I've also only noticed it a few days ago (when looking into that
eval
issue we had), that they're renaming their functions... and didn't really think any further that this might also affect_known_hosts_real()
, but it does … in scop/bash-completion@d311921 they’ve renamed it to_comp_compgen_known_hosts()
.But actually I think this consolidation is quite reasonable for them to do… the many different names and prefixes they've started to use over time were much more messy and likely that people accidentally use such names for something else.
I've already mention that earlier here, I think hat
fzf
might also benefit from such systematic consolidation (e.g. names likeFZF_CTRL_T_COMMAND
are pretty vague (an ideally the keybinding should be configurable and not fixed toCtrl-T
).It may even make sense for
fzf
to integrate in the naming schema used by bash-completion to some extent... e.g. if we'd always use_comp_fzf_*
or so for functions.Anyway I don't see the big deal breaker here?!
It's not that they'd do this every year or so... and that consolidation they do seems to rather make this stable/official now.
APIs do change, or the code is either dead are - less likely - already perfect.
Especially, it's trivially easy to support both (see #3476).
Also, there's already much more complex to read code, that we have now in order to support different versions. The
mawk
-version check or the code to support old bash versions.Even if we would start using more bash-completion functions (e.g. also for the
eval
thing), I'd guess their total number would be rather limited, so I think this is pretty manageable.Similarly, even if I could persuade you to e.g. rename
FZF_CTRL_T_COMMAND
and the likes (which I guess might be a tough job to do ;-) ), it would be easy to fall back to the old names, and e.g. print deprecation warnings for a while.