Better fishfile location support #547

mattmc3 · 2019-07-31T17:00:59Z

Per issue #545, support fishfile living in $fish_config even when $fisher_path is set if desired.

Implementation details:

Added a new _fishfile function
If $fisher_path/fishfile exists and $fish_config/fishfile doesn't, then use $fisher_path/fishfile. Otherwise use $fish_config/fishfile.
Set local variable for fishfile to call _fishfile function

Per issue #545, support fishfile living in $fish_config even when $fisher_path is set if desired.

mattmc3 · 2019-07-31T17:02:08Z

I'll leave the decision to bump the version to you this time @jorgebucaran. No README updates needed in my estimation. The only possible "breaking" behavior would be for people who have $fisher_path set different to $fish_config, have fishfiles in both locations, and are expecting the $fisher_path one to work and get surprised that the $fish_config one does. I think this is likely a non-issue.

jorgebucaran · 2019-10-08T00:55:23Z

@mattmc3 I haven't merged here because I don't really understand why I would need this despite reading #545 again. Could you break this down for me? Thanks.

mattmc3 · 2019-10-08T01:33:10Z

Could you break this down for me?

@jorgebucaran Sure thing.

Problem Statement:
The location of your fishfile if $fisher_path is not set is ~/.config/fish/fishfile. That is a perfectly fine default, and not a problem.

However, the problem comes in if you set $fisher_path. Let's say you set it to something like ~/.config/fisher or ~/.config/fish/fisher. Now, your fishfile will now live at $fisher_path/fishfile (ie: ~/.config/fish/fisher/fishfile) instead of ~/.config/fish/fishfile. That is not ideal.

Placing the fishfile in the $fisher_path directory is an unwanted side effect of setting $fisher_path. This is because your fishfile is a configuration file - something you want to see, not hide away. You may hand edit it, or check it into a git repo when saving your dotfiles. The rest of the contents of $fisher_path is essentially cache - fully re-creatable from your fishfile alone and something you want to hide. In short, moving fishfile to $fisher_path is not desirable because it mingles configuration and cache, and makes an important configuration file unreachable for dotfiles git management without workarounds like symlinking.

The reason someone sets their $fisher_path is because they don't want 3rd party fish plugins to co-mingle with their custom fish config. Setting $fisher_path allows someone to move the fisher managed plugins outside of the rest of their fish config, keeping that area clean. This PR allows for the user to keep their fishfile with the rest of their fish config - in ~/.config/fish/fishfile where it belongs.

Solution

Change the default location of fishfile to not use $fisher_path/fishfile and leave it at ~/.config/fish/fishfile, unless that file already exists at $fisher_path/fishfile. Encourage users to store fishfile in ~/.config/fish/fishfile, and from now on default to that location for new installs.

PR Breakdown

function _fishfile figures out if a fishfile exists in the old location, $fisher_path/fishfile. If so, it uses that so as not to break old fisher setups. Otherwise, it starts storing fishfile in ~/.config/fish/fishfile with the rest of your fish config.
function _fishfile abstracts the location of the fishfile AND the actual name so that you can easily make a change in one place in the future to rename the fishfile to fish_bundle or whatever you want (Rename fishfile to fish_plugins #524).
~~The rest of the changes are to set and use a local $fishfile variable from the result of calling function _fishfile~~
Simply change all references to $fisher_path/fishfile to $fish_config/fishfile
Put in a temporary code block to re-home $fisher_path/fishfile to $fish_config/fishfile if there's not already a $fish_config/fishfile. This code can eventually be removed after a reasonable upgrade period.

mattmc3 · 2019-10-08T02:02:35Z

I do wish @Scrumplex had weighed in on #545 though, just to make sure that looking for $fisher_path/fishfile first still met his needs.

jorgebucaran · 2019-10-14T04:10:39Z

I understand the problem now and fully agree with you @mattmc3. I'd prefer to avoid all the complications and simply default to ~/.config/fish/fishfile without the possibility of configuration.

If you can adjust your PR for that I'll merge this in a heartbeat.

jorgebucaran · 2019-10-20T18:03:20Z

Ping @mattmc3. Any chance you could look at this? 👋

mattmc3 · 2019-10-21T22:44:14Z

I'd prefer to avoid all the complications and simply default to ~/.config/fish/fishfile

@jorgebucaran - Can you affirm my understanding of this statement? I interpret this to mean that you don't want the _fishfilefunction and we're just going back to ~/.config/fish/fishfile. What you're asking will break support on #480. That's okay? If we're agreed, I'll push another commit that simplifies this change. Also, any guidance on how you want to handle people that have their fishfile already in $fisherpath? Doing what you suggest will affect those folks too.

jorgebucaran · 2019-10-22T00:34:40Z

@mattmc3 Yes, that's why I mean. Of course, we can design temporary measures to help people transition. What do you have in mind?

Per discussion here: #547 #545 Fish file is returned from $fisher_path/fishfile to $fish_config/fishfile.

mattmc3 · 2019-10-22T20:54:22Z

fisher.fish

+    # 2019-10-22: temp code, migrates fishfile from old path back to $fish_config
+    if test -e "$fisher_path/fishfile"; and test ! -e "$fish_config/fishfile"
+        command mv -f "$fisher_path/fishfile" "$fish_config/fishfile"
+    end


Let me know if this works for you for fishfile migration @jorgebucaran.

Seems alright.

mattmc3 · 2019-10-22T20:55:37Z

@jorgebucaran - I have pushed a new, simplified commit for re-homing $fish_config/fishfile.

mattmc3 · 2019-10-27T16:28:04Z

ping @jorgebucaran - any additional feedback on this? If I missed the mark on what you were hoping for let me know and I'll take another swing.

jorgebucaran · 2019-10-27T18:40:37Z

@mattmc3 I think we're good to go.

Ping @Scrumplex.

Scrumplex · 2019-10-27T18:41:43Z

I HAVE BEEN SUMMONED. (didn't look into this at all yet, give me a minute 😄)

Scrumplex

Looks good to me. I don't really need the use-case I introduced with #480 anymore. So this change is generally welcome, as I do agree, that putting a config file at the same place as a cache is not a good idea.

Except for my comment there I approve this overall.

Scrumplex · 2019-10-27T18:50:31Z

fisher.fish

@@ -181,7 +187,7 @@ end
 function _fisher_commit -a cmd
    set -e argv[1]
    set -l elapsed (_fisher_now)
-    set -l fishfile $fisher_path/fishfile
+    set -l fishfile $fish_config/fishfile


What's up with this? Didn't we define $fishfile earlier in line 12?

@Scrumplex - The -l flag defines a local variable in the scope of the function, so it needs to be done in both places if we want to use locals. I could convert the one on line 12 in the main fisher function to global (-g). Stylistically, it's probably fine in this project to do it that way - not my typical choice to use globals this way, but if it causes heartburn to define it in two places and a global is preferred, we can do that. @jorgebucaran - do you have a preference here?

Going to agree with @Scrumplex, but only if fishfile can't change. See https://github.com/jorgebucaran/fisher/pull/547/files#r339362195.

jorgebucaran · 2019-10-27T20:29:36Z

fisher.fish

@@ -9,6 +9,7 @@ function fisher -a cmd -d "fish package manager"
    set -g fisher_config $XDG_CONFIG_HOME/fisher

    set -q fisher_path; or set -g fisher_path $fish_config
+    set -l fishfile "$fish_config/fishfile"


@mattmc3 If I get this right, fishfile is defined only once and never changes again. If so, this should be -g.

Solid feedback! I pushed a new commit, which I'm real happy with how it simplifies the whole feature. One global, used everywhere $fishfile needs referenced.

mattmc3 · 2019-10-27T20:43:32Z

@jorgebucaran - I'm happy with this if you are.

mattmc3 · 2019-11-08T21:07:01Z

@jorgebucaran - Anything else I can do to help get this merged?

jorgebucaran · 2019-11-15T16:40:47Z

@neersighted @Scrumplex Could you please update Fisher to get these changes and help me verify if things are running as they should?

@mattmc3 One week later -- is everything working well for you? ;)

mattmc3 · 2019-11-15T17:25:12Z

One week later -- is everything working well for you? ;)

@jorgebucaran - Solidly. My existing config has been rock solid, and I've walked someone through a new fish + fisher setup too since the release - no issues on my end.

Better fishfile location support

be5ee04

Per issue #545, support fishfile living in $fish_config even when $fisher_path is set if desired.

mattmc3 mentioned this pull request Jul 31, 2019

Make path for fishfile configurable to be separate from $fisher_path #545

Closed

jorgebucaran self-assigned this Aug 3, 2019

Re-home fishfile to $fish_config/fishfile

90387fd

Per discussion here: #547 #545 Fish file is returned from $fisher_path/fishfile to $fish_config/fishfile.

mattmc3 commented Oct 22, 2019

View reviewed changes

Scrumplex suggested changes Oct 27, 2019

View reviewed changes

jorgebucaran reviewed Oct 27, 2019

View reviewed changes

Change $fishfile to global variable

99cc745

jorgebucaran assigned mattmc3 Nov 9, 2019

jorgebucaran added Breaking labels Nov 9, 2019

jorgebucaran merged commit 25ae8be into jorgebucaran:master Nov 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better fishfile location support #547

Better fishfile location support #547

mattmc3 commented Jul 31, 2019

mattmc3 commented Jul 31, 2019 •

edited

Loading

jorgebucaran commented Oct 8, 2019

mattmc3 commented Oct 8, 2019 •

edited

Loading

mattmc3 commented Oct 8, 2019

jorgebucaran commented Oct 14, 2019 •

edited

Loading

jorgebucaran commented Oct 20, 2019

mattmc3 commented Oct 21, 2019 •

edited

Loading

jorgebucaran commented Oct 22, 2019

mattmc3 Oct 22, 2019

jorgebucaran Oct 27, 2019

mattmc3 commented Oct 22, 2019

mattmc3 commented Oct 27, 2019

jorgebucaran commented Oct 27, 2019 •

edited

Loading

Scrumplex commented Oct 27, 2019

Scrumplex left a comment

Scrumplex Oct 27, 2019

mattmc3 Oct 27, 2019

jorgebucaran Oct 27, 2019

jorgebucaran Oct 27, 2019

mattmc3 Oct 27, 2019

mattmc3 commented Oct 27, 2019

mattmc3 commented Nov 8, 2019

jorgebucaran commented Nov 15, 2019 •

edited

Loading

mattmc3 commented Nov 15, 2019

Better fishfile location support #547

Better fishfile location support #547

Conversation

mattmc3 commented Jul 31, 2019

mattmc3 commented Jul 31, 2019 • edited Loading

jorgebucaran commented Oct 8, 2019

mattmc3 commented Oct 8, 2019 • edited Loading

mattmc3 commented Oct 8, 2019

jorgebucaran commented Oct 14, 2019 • edited Loading

jorgebucaran commented Oct 20, 2019

mattmc3 commented Oct 21, 2019 • edited Loading

jorgebucaran commented Oct 22, 2019

mattmc3 Oct 22, 2019

Choose a reason for hiding this comment

jorgebucaran Oct 27, 2019

Choose a reason for hiding this comment

mattmc3 commented Oct 22, 2019

mattmc3 commented Oct 27, 2019

jorgebucaran commented Oct 27, 2019 • edited Loading

Scrumplex commented Oct 27, 2019

Scrumplex left a comment

Choose a reason for hiding this comment

Scrumplex Oct 27, 2019

Choose a reason for hiding this comment

mattmc3 Oct 27, 2019

Choose a reason for hiding this comment

jorgebucaran Oct 27, 2019

Choose a reason for hiding this comment

jorgebucaran Oct 27, 2019

Choose a reason for hiding this comment

mattmc3 Oct 27, 2019

Choose a reason for hiding this comment

mattmc3 commented Oct 27, 2019

mattmc3 commented Nov 8, 2019

jorgebucaran commented Nov 15, 2019 • edited Loading

mattmc3 commented Nov 15, 2019

mattmc3 commented Jul 31, 2019 •

edited

Loading

mattmc3 commented Oct 8, 2019 •

edited

Loading

jorgebucaran commented Oct 14, 2019 •

edited

Loading

mattmc3 commented Oct 21, 2019 •

edited

Loading

jorgebucaran commented Oct 27, 2019 •

edited

Loading

jorgebucaran commented Nov 15, 2019 •

edited

Loading