-
Notifications
You must be signed in to change notification settings - Fork 892
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
Add support for fish shell #3108
Conversation
src/cli/self_update/shell.rs
Outdated
let mut path = PathBuf::from(home); | ||
path.push("fish/config.d/rustup.fish"); | ||
vec![path] | ||
} else if let Some(mut path) = utils::home_dir() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for not getting to this earlier.
We generally try to write to all the places that a shell might read. The helper we wrote is idempotent to support this. What would happen if that was done here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, to be honest I've lost all the context since I've opened this PR... Do you mean I should just call rcfiles
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approximately - look at zsh for the typical pattern
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rbtcollins the current version calls rcfiles
here.
Is there anything more that I can do here?
Hm, I've also noticed that the default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new version looks good!
@rbtcollins may I ask why this has been blocked from merging for so long?
Yeah same, is there any reason this can't be merged? |
@sevensidedmarble there's no point in piling on -- please bear with us while we try to increase review capacity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loving the changes/updates
This is an attempt to fix #478.
I'm not sure how to test if it works though, do I have to install local build of rustup in a vm somehow? 🤔