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

Remove eval usage in fish launcher #961

Merged
merged 1 commit into from
Dec 28, 2024
Merged

Remove eval usage in fish launcher #961

merged 1 commit into from
Dec 28, 2024

Conversation

kidonng
Copy link
Contributor

@kidonng kidonng commented Dec 18, 2024

As stated in fish documentation on eval:

If the command does not need access to stdin, consider using source instead.

This has been tested manually on latest fish, though it should be compatible with any relevant version.

@Canop
Copy link
Owner

Canop commented Dec 18, 2024

Thanks

Can any other Fish expert here check it's Ok ? @Lucretiel @offbyone @akx @basbebe

@kidonng
Copy link
Contributor Author

kidonng commented Dec 18, 2024

Code golfing a bit the launcher could even be:

function br --wraps=broot
    function _broot
        broot --outcmd $argv[1] $argv[2..] && source $argv[1]
    end

    _broot (: | psub) $argv
end

Albeit cryptic for those unfamiliar with fish 😅

@Canop
Copy link
Owner

Canop commented Dec 23, 2024

I'm sad no other fish user can check this

@Canop
Copy link
Owner

Canop commented Dec 28, 2024

A side effect of your change is that the temp file is removed only after the execution of the command

@kidonng
Copy link
Contributor Author

kidonng commented Dec 28, 2024

True but practically no difference. Even if source fails for some mysterious reason rm will continue to be run just as fine; if the shell blows up after source then we have much bigger problems than not getting to clean up a temporary file.

@Canop Canop merged commit 23e7f77 into Canop:main Dec 28, 2024
1 check passed
@kidonng
Copy link
Contributor Author

kidonng commented Dec 28, 2024

Thanks!

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.

2 participants