Improve docs and autoinstall behaviour #749
Conversation
This change is for users who commit fish_plugins and plugin sources but
omit fish_variables. On a system where Fisher's universal variables are
unset, most Fisher commands were not working out-of-the-box:
* `fisher install <plugin>` - installs <plugin>, rewrites fish_plugins file with only <plugin>
- if <plugin> is in fish_plugins, same behavior as `fisher update`
* `fisher remove <plugin>` - fails with error: Plugin not installed "<plugin>"
* `fisher update <plugin>` - fails with error: Plugin not installed "<plugin>"
* `fisher update` - fails with error about conflicting files, deletes fish_plugins file
* `fisher list [<regex>]` - returns nothing
As of this commit all Fisher commands work except for `fisher remove`;
Fisher still has no way of knowing which files to remove absent the
universal variable that tracks the files associated to a plugin.
It may make sense to reject calls like `fisher remove <plugin>` if the
`_fisher_<plugin>_files` universal variable is missing. Fisher could
suggest the user run `fisher update` and try again.
This improves speed and allows better automation via config.fish without recursion
I try to fisher update first, in order to read fish_plugins and not overwrite it.
| @@ -82,7 +82,7 @@ function fisher --argument-names cmd --description "A plugin manager for Fish" | |||
|
|
|||
| command mkdir -p $source/{completions,conf.d,themes,functions} | |||
|
|
|||
| $fish_path --command " | |||
| $fish_path --no-config --command " | |||
There was a problem hiding this comment.
As stated in #748 it avoids recursion, making easier to write confs files (not having to test for status is-interactive)
I've checked the command after line 85 and it seems to work fine with fish without config, as it should not require any user customizations
In addition, not reading config when it's not necessary should result in faster execution.
This --no-config flag was added on fish-shell/fish-shell#7921 in order to fix fish-shell/fish-shell#5394, that is, precisely avoid guarding with status --is-interactive
There was a problem hiding this comment.
faster execution
Awesome. 💯
If we start using --no-config (and I think we should) we don't need to add this to the docs.
There was a problem hiding this comment.
Thanks.
Actually, if you look closely, the docs complement the code modification.
Right now the PR docs only check if function fisher exists. This relays on --no-config modification.
If the later were not implemented, the former should include status --is-interactive to avoid infinite recursion.
There was a problem hiding this comment.
@Susensio Could you open a new PR that only includes the --no-config fix? Or modify this one like so. You are welcome to open a new PR to improve the docs if you want.
| @@ -82,7 +82,7 @@ function fisher --argument-names cmd --description "A plugin manager for Fish" | |||
|
|
|||
| command mkdir -p $source/{completions,conf.d,themes,functions} | |||
|
|
|||
| $fish_path --command " | |||
| $fish_path --no-config --command " | |||
There was a problem hiding this comment.
@Susensio Could you open a new PR that only includes the --no-config fix? Or modify this one like so. You are welcome to open a new PR to improve the docs if you want.
| test -e $fish_plugins && set --local file_plugins (string match --regex -- '^[^\s]+$' <$fish_plugins) | ||
| set --query _fisher_plugins || set --universal _fisher_plugins $file_plugins |
There was a problem hiding this comment.
@Susensio What is this change about?
There was a problem hiding this comment.
This is a merge from #746 that I added into my personal fork, I did not know it would modify this PR
There was a problem hiding this comment.
Let me split this into multiple PRs so you can decide better
There was a problem hiding this comment.
The --no-config part is no longer needed. I thought that it was intended to avoid status is-interactive but it is more than that.
The subshell fisher starts to parallelize installation does not need to read config, as it seem (at the moment) to only write to temp folders and such. It wouldn't hurt to add this, but the improvement in speed is negligible so I think it's safer and simpler to let it be as is.
|
Simplified and fixed in #756 |
#748