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.

Sign up for GitHub

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

Enable completions and add description for lfcd.fish and simplify the code #1503

Merged
merged 7 commits into from
Dec 11, 2023
Merged

Enable completions and add description for lfcd.fish and simplify the code #1503

Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3d74fee
Enable completions and add description for lfcd.fish
postsolar Nov 18, 2023
7c40601
Simplify lfcd.fish
postsolar Nov 18, 2023
7038f6a
lfcd.fish: unquote $argv
postsolar Nov 18, 2023
7537fb1
lfcd.fish: further simplify
postsolar Nov 18, 2023
1864cd0
lfcd.fish: cd only on clean exit
postsolar Nov 20, 2023
4c19fe8
lfcd.fish: further simplify and add a coment on quoting semantics
postsolar Dec 4, 2023
bd330f5
lfcd.fish: amend comment style
postsolar Dec 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 2 additions & 12 deletions etc/lfcd.fish
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,7 @@
#
# You may put this in a function called fish_user_key_bindings.

function lfcd
set tmp (mktemp)
function lfcd --wraps="lf" --description="lf - Terminal file manager (changing directory on exit)"
# `command` is needed in case `lfcd` is aliased to `lf`
command lf -last-dir-path=$tmp $argv
if test -f "$tmp"
set dir (cat $tmp)
rm -f $tmp
if test -d "$dir"
if test "$dir" != (pwd)
cd $dir
end
end
end
cd (command lf -print-last-dir $argv)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated it 👍

Copy link
Contributor Author

@postsolar postsolar Nov 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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"?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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 exist

Alternatively, 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)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@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)"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ilyagr sorry, I don't follow: are you suggesting to introduce a check for whether the resulting dir variable is empty?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@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)"

Copy link
Collaborator

@ilyagr ilyagr Dec 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@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 cd to fail if lf prints nothing to stdout

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joelim-work @ilyagr thank you for clarifying! I updated the code and added a comment

end