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

Add a fish_delta helper function #9255

Merged
merged 2 commits into from
Oct 19, 2022
Merged

Add a fish_delta helper function #9255

merged 2 commits into from
Oct 19, 2022

Conversation

faho
Copy link
Member

@faho faho commented Oct 3, 2022

Description

This goes through $fish_function_path/$fish_complete_path and checks which files you have that aren't in the stock fish install. So it helps you to figure out which functions and completions you've overridden.

For instance, on my system:

> fish_delta -fd | head -n 5
New file: /home/alfa/.config/fish/test/functions/__fish_supports_cursor.fish
New file: /home/alfa/.config/fish/test/functions/_fish_systemctl.fish
New file: /home/alfa/.config/fish/test/functions/aur.fish
New file: /home/alfa/.config/fish/test/functions/fish_describe_machine.fish
New file: /home/alfa/.config/fish/test/functions/fish_set_cursor.fish
> fish_delta --no-completions --no-new --no-diff
File /home/alfa/.config/fish/functions/fish_clipboard_copy.fish differs from /usr/share/fish/functions/fish_clipboard_copy.fish
File /home/alfa/.config/fish/functions/fish_greeting.fish differs from /usr/share/fish/functions/fish_greeting.fish
File /home/alfa/.config/fish/functions/fish_prompt.fish differs from /usr/share/fish/functions/fish_prompt.fish

(and without "-d"/"--no-diff" it would show you the diff on changed files, with automatic paging - but that's awkward to demonstrate here)

One wrinkle is that it'll only show function files - if you've e.g. overridden a function in config.fish, we won't be showing it. I've tried working on that, but it's awkward considering multi-function files like fish_git_prompt.fish.

I think it's still useful, it'll allow me to figure out if I have a completion I'm testing (hence the test/completions directory!) and need to merge.

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • [N/A] Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.rst
  • Should we ignore vendor stuff, by default?
  • Should we ignore new files by default?

@faho faho added this to the fish-future milestone Oct 3, 2022
@faho
Copy link
Member Author

faho commented Oct 4, 2022

Okay, ignoring new files makes more sense - they don't change anything, they aren't relevant for debugging etc, and there may be a lot of them.

I'm still not happy with the vendor dirs - there are three ways to handle them:

  1. Treat them as "user" customization. Anything they do counts as "changed" or "new" compared to what we ship
  2. Treat them as "default" - anything the user has of the same name as them counts as "changed"
  3. Ignore them, like we do the generated completions - anything the user has of the same name counts as "new" (unless it's also in our dirs, in which case it might be changed)

In a certain sense, number 1 is true. These aren't things we ship, they are customizations compared to a stock fish.

But in another sense, that's entirely not the same as a user overriding things. I have a lot in these directories simply because my distro packages ship stuff there! So it's "default" according to what my system ships.

So number 3 feels like the least annoying option, possibly with a flag to switch to 1. (but tbh our vendor path logic is overcomplicated, especially with all the $XDG_DATA_DIRS stuff, and there's an actual possibility we end up getting these paths wrong)

@krobelus
Copy link
Contributor

krobelus commented Oct 6, 2022

this is intriguing, seems very useful.

I'm still not happy with the vendor dirs

On a first glance, 2 or 3 sound like viable options.
I certainly would like to simplify the vendor loading logic, somehow..

share/functions/fish_delta.fish Outdated Show resolved Hide resolved
share/functions/fish_delta.fish Outdated Show resolved Hide resolved
end

not set -qx LESS
and set -xf LESS $lessopts
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if theres a good way to extract the paging logic.
A function will probably not work because we need to pipe into it.. maybe we can create a function that outputs the pager command

@faho
Copy link
Member Author

faho commented Oct 6, 2022

In fact I probably need to see if --color=always needs to be checked.

Okay I wasn't in the mood to do feature detection so I just added coloring via string replace. I await your screams.

@faho
Copy link
Member Author

faho commented Oct 7, 2022

Alright I implemented a "--vendor=" option to allow picking how these are counted.

By default they are counted as "default" (or "stock") files.

Note: This necessitates new global variables set via share/config.fish - along with e.g. $__fish_data_dir we now have $__fish_vendor_functionsdirs, $__fish_vendor_completionsdirs and $__fish_vendor_confdirs. These are set to the directories we used on startup.

The alternative is to redo the logic for finding the vendor files in fish_delta, which appears worse.

It just feels kind of awkward to make that change here, in a semi-related PR, but on the other hand I also don't love doing a separate PR and threading that in here?

@mqudsi
Copy link
Contributor

mqudsi commented Oct 7, 2022

I think it helps to decide what the biggest use case for this would be in determining how to handle vendor dirs.

e.g. is this for the user's discoverability or for us to be able to determine if the user is running a stock config or not (we could ask them to run this when they file an issue and it appears that it's a config-related problem).

@faho
Copy link
Member Author

faho commented Oct 7, 2022

e.g. is this for the user's discoverability or for us to be able to determine if the user is running a stock config or not (we could ask them to run this when they file an issue and it appears that it's a config-related problem).

As far as I'm concerned: Both.

One use case is finding out "what have I changed" - as an example I, personally, used previous evolutions of this to figure out what stuff I have left to commit, because I like test-driving things in my personal config for a while.

Another is finding out "what is changed over the stock configuration". This is both for debugging - bug reports to us could include something like this[0] - and for figuring out what sort of stuff differs from a stock fish so you can redo it on other systems.

Both of these necessitate treating vendor dirs differently - in the former, you don't really care about the separation between "the fish project's stuff", so it should all be counted together. This should be the most common and so it is the default mode of operation.

In the latter, you want to know what changed over "the fish project's stuff", so this is "--vendor=user" mode.

The idea behind "--vendor=ignore" was that you'd use it if the vendor stuff is annoying. You don't really choose what gets installed there and removing stuff there is frowned upon (because it's installed from packages and will just be overwritten on update), so you might end up with a bunch of vendor stuff that you don't care about.


[0]: But note that people love their aliases in config.fish and this won't help with that, so it's not a panacea.

@faho
Copy link
Member Author

faho commented Oct 18, 2022

Okay, this now looks at config files as well.

Like the commit message calls out, it will count them as "changed" even if they don't shadow anything. This means you'll easily see all your snippets and config. On the other hand it means you'll see a bunch.

Let's see how that shakes out - this is easy enough to change later because it's not really a "script" thing.

@mqudsi
Copy link
Contributor

mqudsi commented Oct 18, 2022

Do you want to add it to the changelog right now, while it's still fresh in your mind?

@faho
Copy link
Member Author

faho commented Oct 18, 2022

Do you want to add it to the changelog right now, while it's still fresh in your mind?

I find doing changelog in the PR only leads to annoying merge conflicts.

faho added 2 commits October 19, 2022 19:46
This lets us query them later, which helps with fish_delta
This helps figuring out which functions, completions and config you've overridden.
@faho faho merged commit 054f9ba into fish-shell:master Oct 19, 2022
@faho faho modified the milestones: fish-future, fish 3.6.0 Oct 19, 2022
@faho faho deleted the fish_delta branch October 26, 2022 10:20
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants