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

Changed fisher_data path and rm command #37

Closed
wants to merge 2 commits into from

Conversation

jsmitley
Copy link

Moved fisher_data path from .config to .local/share to match latest fisher release
Changed rm to command rm so it doesn't try to use a fish rm function

@jsmitley jsmitley changed the title Changed fisher path from .config to .local/share Changed fisher_data path and rm command Sep 21, 2020
@reitzig
Copy link
Owner

reitzig commented Oct 12, 2020

We should probably use XDG variables like fisher. 🤔

Will look at this when I next touch the project. Won't be tomorrow, sorry. Thanks for the ping!

Change fisher install command
@jsmitley
Copy link
Author

jsmitley commented Dec 1, 2020

Good point, I made that change now. Please look when you have a chance. Thanks!

README: Update install command to fisher 4
@reitzig reitzig changed the base branch from master to dev January 22, 2021 02:39
conf.d/sdk.fish Outdated
@@ -44,7 +47,7 @@ function __fish_sdkman_run_in_bash
env | grep -i -E \"^(`echo \${SDKMAN_CANDIDATES_CSV} | sed 's/,/|/g'`)_HOME\" >> $pipe;
echo \"SDKMAN_OFFLINE_MODE=\${SDKMAN_OFFLINE_MODE}\" >> $pipe;
echo \"SDKMAN_ENV=\${SDKMAN_ENV}\" >> $pipe" # it's not an environment variable!
set bashDump (cat $pipe; rm $pipe)
set bashDump (cat $pipe; command rm $pipe)
Copy link
Owner

Choose a reason for hiding this comment

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

Can you please explain which problem this change solves? 🤔

Copy link
Author

@jsmitley jsmitley Feb 22, 2021

Choose a reason for hiding this comment

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

Sorry I'm late with this! I have a separate command in my config that overrides rm (calls trash instead), which breaks this call. If I use the command to call rm, it ensures that any overrides for that command are disregarded and it seems to get the same desired result.

Copy link
Author

Choose a reason for hiding this comment

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

@reitzig Any thoughts on this? I don't know of a more fish-y way to handle this particular command override 🤷

Copy link
Owner

@reitzig reitzig Jun 25, 2023

Choose a reason for hiding this comment

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

I don't think it's feasible to look out for overrides of "basic" commands. :/ See also #23.

I would basically have to prefix all tool calls with command -- and inevitably break some setup somewhere by doing that anyway.

FWIW, this is one reason why I hate writing shell scripts instead of "real" programs. In this instance, it can't be helped.

@rmartine-ias
Copy link

Changing rm to command rm also fixes the issue I'm having, where I aliased rm to rm -i. Was seeing something like this every time I ran sdk help:

rm: remove regular file '/var/folders/ht/q8z65bc15mx7gfzx5t3s7_mr0000gq/T/tmp.jFun2SabOO'?

@reitzig
Copy link
Owner

reitzig commented Jun 25, 2023

Sorry for the long silence.

Moved fisher_data path from .config to .local/share to match latest fisher release

Somehow, only a README change is left on this PR. I can't seem to remember what this part was about. Is there still an issue?

Ah, got it. This is about the line that sets __fish_sdkman_noexport_init. The path I hard-coded there was based on where fisher 3 installed plugin sources; fisher 4 uses the config dir of fish directly, unless overridden. We now use their code to determine where to put the auxiliary script. Thanks for the pointer, even though I didn't end put merging your PR! 🙏

@reitzig reitzig closed this Jun 25, 2023
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.

4 participants