Skip to content

Make fish_add_path more transparent#10532

Merged
faho merged 2 commits into
fish-shell:masterfrom
faho:add-path-autoverbose
Jun 6, 2024
Merged

Make fish_add_path more transparent#10532
faho merged 2 commits into
fish-shell:masterfrom
faho:add-path-autoverbose

Conversation

@faho
Copy link
Copy Markdown
Member

@faho faho commented May 28, 2024

Description

fish_add_path has a pretty tricky dual nature:

  • It's usable in config.fish - you throw it in there and it does The Right Thing
  • It's usable in the commandline

These two have different needs - scripted use needs it to be as quiet as possible. If a path wasn't added because it's already included, that's probably because it already ran. If a path wasn't added because it's a file, that's probably because it was a directory on another system.

Interactive use, on the other hand, suffers a bit from a lack of feedback - you do something and it either silently returns true or false, without explaining why.

We have "verbose" mode - fish_add_path -v, but that's something people need to know about, and it currently does not explain enough.

So, what this does is:

  1. It makes verbose mode more informative by printing a message for each path that was eliminated and also if no path was left
  2. (the spicy part) It automatically enables verbose mode if fish_add_path is used interactively, from the commandline - if you run fish_add_path /foo at your prompt, you'll get messages

That latter part is a bit naughty - numerous CLI guidelines don't like this. But I do believe it's helpful, and I don't really see an alternative - anything else requires adding a flag to get messages interactively, or adding a flag to suppress them noninteractively.

I see people on the internet fail regularly by trying to add a file instead of the directory it contains:

fish_add_path /opt/bin/texlive # <- should be /opt/bin!

and we currently have no warning about this - it will just silently return false. You'll have to run fish_add_path -v /opt/bin/texlive to get "Skipping path because it is a file instead of a directory".

Adding this means they'll get the feedback, which should make their mistake obvious - add the directory. (an alternative for this specific case is to automatically dirname any file, but the lack of information hurts in other cases as well)

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.rst

faho added 2 commits May 28, 2024 11:26
fish_add_path can be used either interactively, in the commandline,
or in config.fish. That's its greatest strength, it's a very
DWIM-style command.

One of the compromises that entails, however, is that it can't really
be very loud about what it does. If it skips a path, it can't write a
warning because it might be used in config.fish.

But it *can* if it's used interactively. So we try to detect that case
and enable verbose mode automatically.

That means if you do

```fish
fish_add_path /opt/mytool/bin/mytool
```

it may tell you "Skipping path because it is a file instead of a
directory:".

The check isn't perfect, it goes through status current-command and
isatty, but it should be good for most cases (and be false in config.fish).
One issue with fish_add_path at the moment is that it is sometimes a bit too intransparent.

You'll try to add a path, but it won't appear - was that because it wasn't a directory,
or because it doesn't exist, or because it was already included?

If it isn't usable after, did fish_add_path not add it because of something or did something *else* remove it?

So we give more explanations - "skipping this because it's a file", "not setting anything because no paths are left to add", ...
@faho faho added this to the fish next-3.x milestone May 28, 2024
@zanchey
Copy link
Copy Markdown
Member

zanchey commented May 28, 2024

I agree this is helpful.

@mqudsi
Copy link
Copy Markdown
Contributor

mqudsi commented May 30, 2024

I think this is a good idea, but maybe for different reasons?

In particular:

I see people on the internet fail regularly by trying to add a file instead of the directory it contains:

I would like to see this error out regardless if it's being used interactively or in config.fish. After all, someone experimenting by adding it to config.fish isn't necessarily going to be trying it interactively (or even realizing that it's an option to do so).

@faho
Copy link
Copy Markdown
Member Author

faho commented May 30, 2024

I would like to see this error out regardless if it's being used interactively or in config.fish

Unfortunately that would run into the same problem we had when we errored out setting $PATH with an invalid path - people sync their config to other machines, and then ssh breaks.

That's why we removed that, and why fish_add_path currently does not print an error, especially because these things aren't really exceptional - the path you gave isn't usable as a component to $PATH, so we're just not adding it.

@mqudsi
Copy link
Copy Markdown
Contributor

mqudsi commented May 30, 2024

The specific case of a path existing but being a file is distinct from the path not existing at all, and I think the odds of the path being a file on one system but a directory on another is relatively unlikely?

@faho
Copy link
Copy Markdown
Member Author

faho commented May 30, 2024

The failure mode here is too awkward to print messages in rare cases. Rare messages aren't a good idea because then it's hard to know that there's sometimes a message.

I don't want to break things for people who do everything right (just copy your config to a different machine which happens to have a different file structure) in favor of people who did something wrong - try to add a file to $PATH, which is clearly broken (that's just not how $PATH works).

By contrast in interactive use there's no breakage, it's only upside. So that's a niche we can work in.

For people who aren't hardcore users, I would also assume that they'd be more likely to just run fish_add_path interactively, and even for those that don't I would assume they'd run it interactively before as a test and then add it to a config.fish they might keep in version control.

@mqudsi
Copy link
Copy Markdown
Contributor

mqudsi commented May 31, 2024

As I said, I'm not against the feature you propose. I am just questioning if there isn't more we can do.

Maybe I am missing something but I just cannot see a reasonable "correct case" you speak of that would involve a path sometimes being a directory on some machines and sometimes being a file on others. The typical Unix hierarchy both by definition and by convention would make it, to my mind, extremely improbable.

@faho
Copy link
Copy Markdown
Member Author

faho commented Jun 5, 2024

Maybe I am missing something but I just cannot see a reasonable "correct case" you speak of that would involve a path sometimes being a directory on some machines and sometimes being a file on others

It's not whether it's incorrect or not that these systems differ like that. The systems differ in that way, and then the trigger is something as innocent as

scp config.fish user@host

and suddenly scp is broken. Even with a perfectly well-written config.fish that doesn't print anything, and doesn't have anything that someone would think prints anything (and doesn't on their system, and wouldn't on a system with another fish version)! That's why sometimes sending messages in config.fish is a bad idea, and why we're not doing that.

@mqudsi
Copy link
Copy Markdown
Contributor

mqudsi commented Jun 5, 2024

I guess that's a fair enough reason. Thanks.

@faho faho merged commit f59cdfa into fish-shell:master Jun 6, 2024
@faho faho deleted the add-path-autoverbose branch June 6, 2024 15:13
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Jun 9, 2025
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