Enable completions and add description for lfcd.fish and simplify the code #1503
Conversation
A simple addition of `--wraps=lf` to the function definition will enable the standard lf completions to be used for `lfcd` function as well.
|
This will only help if you manually set up fish completions for |
postsolar
changed the title
|
Now that there are more changes, I can't review it as immediately. |
Depends on the installation method. For example Arch packages include completions so they're automatically enabled on install.
Yep, no worries. Thank you. |
|
I have left a couple of comments. Feel free to correct me on anything - I don't have much experience with |
etc/lfcd.fish
Outdated
| end | ||
| end | ||
| end | ||
| cd (command lf -print-last-dir $argv) |
There was a problem hiding this comment.
What would happen if lf didn't print anything to stdout, e.g. crashed? I wouldn't want to cd to the home dir in that case.
Other than that, this lgtm; I think getting an error from cd when the dir is not valid is fine.
There was a problem hiding this comment.
Although something like lfcd --doc or lfcd -log=/dev/stdout produces errors, do you think it falls under "I think getting an error from cd when the dir is not valid is fine"?
There was a problem hiding this comment.
The updated code looks better to me. Apart from not cd-ing on failure, I'm more interested in that the following does not cd you to the home dir:
🐟 set -g outdir ""
🐟 cd $outdir
cd: Empty directory '' does not existAlternatively, you could insert a check that $outdir is not empty. But, at that point, you could just again check whether the dir exists. I think I'm OK either way.
Thank you!
As an aside, I checked that both of the following cd you to the home dir, so the fix is useful.
cd (printf "")
cd (printf ""; false)There was a problem hiding this comment.
@ilyagr Good catch about cd-ing back to the home directory in case lf crashes, I didn't consider that. Just out of curiosity, would it not be better to quote the command substitution (i.e. "$(...)"), to ensure the output is a blank string instead of an empty list?
I found the following commands result in a cd: Empty directory '' does not exist error:
cd "$(printf '')"
cd "$(false)"There was a problem hiding this comment.
@ilyagr sorry, I don't follow: are you suggesting to introduce a check for whether the resulting dir variable is empty?
There was a problem hiding this comment.
@postsolar I think the point @ilyagr is trying to make is that under no circumstances should lfcd change to the home directory in the case that lf crashes or otherwise prints nothing. I think a local variable is not needed and the below should work:
cd "$(command lf -print-last-dir $argv)"There was a problem hiding this comment.
@postsolar, I'm sorry I missed your question. I think @joelim-work covered what my concern was. I also believe the command he suggested should work fine. Thanks Joe!
One other thing I would add is a comment before that line, along the lines of:
The quotes cause
cdto fail iflfprints nothing to stdout
There was a problem hiding this comment.
@joelim-work @ilyagr thank you for clarifying! I updated the code and added a comment
joelim-work
dismissed
their stale review
Dismissing stale review (forgot out possibility of lf crashing
etc/lfcd.fish
Outdated
| end | ||
| end | ||
| function lfcd --wraps="lf" --description="lf - Terminal file manager (changing directory on exit)" | ||
| # `command` is needed in case `lfcd` is aliased to `lf`, |
There was a problem hiding this comment.
Minor nit: Instead of , and quotes, I'd start another sentence and write . Quotes.
There was a problem hiding this comment.
Indeed, looks better with a new sentence. I also now think the wording "cause cd to fail" is a bit vague (without context it might not be clear why exactly this is desired) so expanded on this bit too.
There was a problem hiding this comment.
Thanks! I do now have an even more minor nit: if we focus on not changing the directory, I'd say:
Quotes will prevent
cdfrom changing the directory iflfprints nothing to stdout due to an error.
I also think that I mildly prefer talking about cd failing, since the user will probably see the error message in that case.
But it's pretty much OK as is, as well.
A simple addition of
--wraps=lfto the function definition will enable the standard lf completions to be used forlfcdfunction as well.I also added a description to the function, so that when the used types
lfand hits tab, a brief description of what the function does is show. I simplified the code to just bind a var to stdout instead of creating a temp file, then read this var and cd to the selected directory.