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

Simple way to add to the path #6960

Closed
boxed opened this issue Apr 30, 2020 · 11 comments
Closed

Simple way to add to the path #6960

boxed opened this issue Apr 30, 2020 · 11 comments

Comments

@boxed
Copy link

boxed commented Apr 30, 2020

(This has been discussed in #527 previously)

I can never remember how to set the path persistently in fish, When I google it I end up at issue #527 but the discussion there is a bit confused. I suggest two things:

  1. Add a command add_to_path that is idempotent and validates the path.
  2. Edit the top comment of How to set $PATH persistently? #527 or the title to point out this new way so people who end up there from google get the right information. Google results changes too slowly!

I have written a draft of an implementation of add_to_path:

#!/usr/bin/env fish
if argparse -n 'fish-add-user-path' -N 1 'h/help' -- $argv
    ;
else
    exit
end

function add_to_path_if_not_there
    set exists 0

    # normalize path to end with /
    switch $argv
    case "*/"
        set new_path $argv
    case "*"
        set new_path "$argv/"
    end

    for x in (string split " " $fish_user_paths)
        if test "$x" = "$new_path"
            set exists 1
            break
        end
    end

    if test "$exists" = "0"
        set -Ux fish_user_paths $fish_user_paths $new_path
    end   
end

# validate that paths exists
for x in (string split " " $argv)
    if test -d $x
        ;
    else
        echo "Path '$x' does not exist"
        exit
    end
end


for x in (string split " " $argv)
    add_to_path_if_not_there $x 
end

This is my first fish program so please help me in making it suck less :P

ping @jonlorusso @knightsamar @kegesch @raunyoliveira-hotmart @i-am-the-slime @samhh @brianmhunt @radoslawc @cumulotimbus @AnotherCoolDude and many more that have seemed to think this is a good idea.

@faho
Copy link
Member

faho commented Apr 30, 2020

Please don't ping tons of people just because they commented once years ago on an issue, it is rude.

for x in (string split " " $fish_user_paths)

That's not how fish does this. $fish_user_paths is a list, so it is already split correctly. Splitting again is wrong. That's one of the major differences between fish script and posix script.

Just do for x in $fish_user_paths.

The same for the other splits.

Tho... don't do for x in $fish_user_paths at all, we have contains.

Also $exists is unnecessary, you can just add directly.

Also don't export $fish_user_paths, it is already universal.

Also we used to check for directory existence and that turned out to be generally not what you want.

So this boils down to, roughly (and untested):

function fish_add_to_path
    set -q fish_user_paths; or set -U fish_user_paths
    for path in $argv
        switch $path
             case "*/"
                 : 
             case "*"
                 set path $path/
         end
         contains -- $path $fish_user_paths; or set -a fish_user_paths $path
    end
end

If you do it interactively, and you have already set $fish_user_paths, the only thing you really need is that last line. Which is why we've been reluctant to add this, because it's a shortcut for something where the building blocks should be understood.

@faho faho added the RFC label Apr 30, 2020
@boxed
Copy link
Author

boxed commented Apr 30, 2020

Might be rude, might be kind. I know I want to be pinged. 🤷‍♂️ The problem here is that the old ticket was closed hard so watchers there don't translate over. One could argue that is rude. Let's drop that though, since it's not really relevant :)

Also we used to check for directory existence and that turned out to be generally not what you want.

That seems very counter intuitive. The idea of this feature is to have something very user friendly, and having things silently fail because you made a copy paste error or spelling error isn't user friendly at all. I think it's much better to validate the path, and if you want to add a (currently) not valid path, have a flag for that and in the error message inform of that flag. Seems like a pretty great UX to me.

Wow, yea I suspected my code was overly complex and bad, but the difference here was bigger than I thought :) Thanks for the improvement! I still think the (optional but default) validation is an important path, so that would need to be added.

Which is why we've been reluctant to add this, because it's a shortcut for something where the building blocks should be understood.

Sorry, didn't understand that part. What are the building blocks that I need to understand when I just want to add something to my path? I don't see why this couldn't be abstracted away and me not needing to understand the parts at all...

@faho
Copy link
Member

faho commented Apr 30, 2020

That seems very counter intuitive. The idea of this feature is to have something very user friendly, and having things silently fail because you made a copy paste error or spelling error isn't user friendly at all.

People will add this to config.fish, and they will share config.fish across machines, and they will not silence the error - because it worked on the machine they tried it on.

Misspelling doesn't really happen because people run config.fish, and when it doesn't work it tends to be noticed quickly.

We've been through this, and experience has shown that not checking is, overall, better. Even if we did, adding a flag would be entirely unnecessary because you could just silence stderr.

Sorry, didn't understand that part. What are the building blocks that I need to understand when I just want to add something to my path? I don't see why this couldn't be abstracted away and me not needing to understand the parts at all...

$PATH is a list. So is $fish_user_paths. When you want to add something to a list, you can use set. When you want to check if something is in a list, you can use contains. When you want to do something if something else fails, you can use or.

So, adding something to $fish_user_paths if it isn't in there boils down to:

contains -- $path $fish_user_paths; or set -a fish_user_paths $path

The nice thing about this is that you can choose yourself if you want to append it or prepend it (change the "-a" to a "-p") and whether you want to set fish_user_paths as a universal or a global.

If you understand this, you can easily write it yourself. If we abstract it, we either need to reintroduce those options or if you want one of them you need to go back to the real thing again.

"Teach a man to fish" and all that.

@boxed
Copy link
Author

boxed commented Apr 30, 2020

People will add this to config.fish, and they will share config.fish across machines, and they will not silence the error - because it worked on the machine they tried it on.

Good point. I still don't agree even if I understand the argument. Letting bugs silently pass isn't a good strategy in my experience, and I don't see why shell scripts should violate this rule. "It works on my machine" is a classic programmer faux pas and you aren't solving it by silent failure, you're making it worse.

Misspelling doesn't really happen because people run config.fish, and when it doesn't work it tends to be noticed quickly.

There is a huge difference in speed of discovery between an error when you make the mistake and spending minutes staring at a path that looks right but isn't.

We've been through this, and experience has shown that not checking is, overall, better

My 20 years of work experience is making that sound really strange.

"Teach a man to fish" and all that.

I use fish because it's so much better using interactively. I will touch the scripting only as an absolute last resort. I believe my type of usage is something that should be considered. Learning all those abstractions and the types of various things etc etc seems like a unnecessary thing to push on people who just want to use this extremely basic feature.

The linked old issue with tons of people saying the same or similar seems to confirm this hypothesis I think.

I guess there is some argument here to what the audience you are trying to reach is too.

@ridiculousfish
Copy link
Member

ridiculousfish commented Apr 30, 2020

Thanks for filing, this issue is much more focused. I know that $PATH is tricky. It's a great observation that #527 is where people land from google, and I will figure out a way to add an authoritative answer at the top.

I like the idea of this function - fish caters to casual shell users and this is evidently one of their pain points. As faho points out the implementation can be simple, just append to $fish_user_paths. I also like the idea of warning when invoked with an argument that is not currently a readable directory, and for redirecting stderr to be the way to silence it.

@boxed Are you willing to make a PR? In it you could update the docs and add the file to share/functions/

@boxed
Copy link
Author

boxed commented Apr 30, 2020

Absolutely!

To be clear the spec is:

  • if already on path just exit success
  • print warning to stderr if path doesn't exist but continue anyway
  • append to path

If you need it first do it another way? Or do we do a flag? Another function add_path_first?

@ridiculousfish
Copy link
Member

That spec looks good, except I would say it should prepend to $PATH, so that new entries have priority. set -U -p fish_user_paths $entry to prepend.

@zanchey zanchey added this to the fish-future milestone May 1, 2020
@charego
Copy link
Contributor

charego commented May 1, 2020

This should be documented. Potential spots:

Naming suggestion: fish_add_path. There could still be confusion since:

  • the name is missing "user" despite it updating "fish_user_paths"
  • it kinda sounds like an appending operation

@boxed
Copy link
Author

boxed commented May 19, 2020

@ridiculousfish what about the name, should we have a fish_ prefix?

faho added a commit to faho/fish-shell that referenced this issue May 21, 2020
This is a function you can either execute once, interactively, or
stick in config.fish, and it will do the right thing.

Some options are included to choose some slightly different behavior,
like setting $PATH directly instead of $fish_user_paths, or moving
already existing components to the front/back instead of ignoring
them, or appending new components instead of prepending them.

The defaults were chosen because they are the most safe, and
especially because they allow it to be idempotent - running it again
and again and again won't change anything, it won't even run the
actual `set` because it skips that if all components are already in.

Fixes fish-shell#6960.
@ridiculousfish
Copy link
Member

....yea, I think a fish_ prefix is warranted

@faho faho closed this as completed in 9354dd6 May 29, 2020
@zanchey zanchey modified the milestones: fish-future, fish 3.2.0 May 31, 2020
@boxed
Copy link
Author

boxed commented Jun 9, 2020

Awesome work @faho ! 🙇‍♂️

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants