Improve interactiveness checks #3449
Conversation
|
@junegunn Any idea why the test suite fails here? I've tried[0] to run the tests manually on some host of mine (on the branch that I want to get merged): The ones that failed here on GH seemed to work when running the suite locally, but others failed (though they seem unrelated to my commits?): Thanks, [0] btw: would be nice if e.g. |
|
Did some more checks, i.e. ran the test suite on master (which results in at least 3 failed tests for me)... and then again on my branch, where tests seem to be flaky: sometimes I also get just the 3, sometimes more. So maybe the test failures have nothing to do with my commits. |
calestyo
force-pushed
the
improve-interactiveness-checks
branch
2 times, most recently
from
1364ace
to
c3826bf
Compare
|
All fish tests are failing even after a retry. You might want to check if the fish version on GitHub actions is compatible with your change. |
Uhm... I've never used github actions... running the tests locally now gives me just: But these seem to happen also on It's really weird. |
This should keep the code more readable, be less error prone (accidentally doing something outside the if-block and aligns the code with what’s already done for zsh. `0` is returned, because it shall not be considered an error when the script is (accidentally) sourced from a non-interactive shell. If executed as a script (rather than sourced), the results are not specified by POSIX but depend on the shell, with bash giving an error in that case. Signed-off-by: Christoph Anton Mitterer <[email protected]>
The shell execution environment shouldn’t be modified at all, when called from a non-interactive shell. It shall be noted that the current check may become error prone for bash, namely in case there should ever be a differentiation between `i` and `I` in the special variable `-` and bash’s `nocasematch`-shell-option be used. Signed-off-by: Christoph Anton Mitterer <[email protected]>
calestyo
force-pushed
the
improve-interactiveness-checks
branch
from
c3826bf
to
fdd22da
Compare
I want to keep it because I think the potential benefit outweighs the cost. |
calestyo
force-pushed
the
improve-interactiveness-checks
branch
2 times, most recently
from
0d3a89d
to
c448711
Compare
|
@junegunn Well no idea what's going on with the test system ^^ Since you've anyway rejected the idea of dropping the useless checks in the completion files I've dropped 1364ace. And since something is fishy with fish (pun intended), I've now dropped 0d3a89d. No idea why it makes the tests fail, it works locally. Maybe the |
|
Anyway... please review and merge :-) |
I don't understand why you keep saying it's useless. Not every package manager treats the file differently so that it's automatically loaded. There are many package managers that provide fzf, and how many have you tested? For example, Homebrew, the most popular plugin manager on macOS doesn't install the file in a different location. Users are expected to source the file from their shell configuration files. You could have at least |
Well it's what I tried to argue in the original message of this PR.
If (1) then one can a) reasonable expect that people read the If (2) then by all means, if the file's placed in a directory where it would be sourced for non-interactive shells: fix the package. And if it doesn't install it, well it probably kinda should (at least if there's a way for users to disable it for them). Anyway... I don't want that to become a useless argument... it's mostly a matter of personal taste, which safeguard is still justified and which is too much. I guess you'd also say that checking for bash is overkill (the user could source all these files from dash)... it just happens that I personally feel, that it's also too much for the completion files. Though as I've indicated in the PR, I can also live with it. :-D |
|
Oh, and thanks again for merging :-) |
Why three? There are two. If you're counting the one from a package manager, that's the code we don't maintain and can't control. If we are going to remove a redundant check, we will remove the one that install script injects and make the completion file self-contained, easier to bring around and use.
You're making up an imaginary scenario and comparing it with a real problem people had. Please
Easier said than done. |
I was referring to the loading when
I believe you that people had problems. :-) It's like every now and then someone posts at the cryptsetup mailing list that he's lost his password has no backups and what he can do to get his data back (nothing). I mean you cannot protect people from everything. Anyway... makes no sense to discuss this further, so lets focus on the other open PRs or new features (actually I'm thinking of a few) 😍 |
No, we can't.
But in this case, we can help them with a small safety device with virtually zero overhead, so why not? |
As said... it's really just a matter of taste, and not that big issue. |
This should fix #3445 and bring some further minor cleanups.
As for the last commit of this series, which removes the interactiveness checks from the completion scripts and your comment:
over in #3445:
I'd guess most people will install software via some form of packaging, which takes care of that - after all that's what the whole distros-system in *nix/BSD/Linux is there for.
If someone installs manually from the sources, well than I think one can reasonably expect him to know what he's doing - or at least to live with it, when doing stupid things.
After all, we can only do so much to prevent people from shooting themselves.
Checking for interactiveness in the key-binding scripts is IMO still some reasonable effort, because users are expected to actually source them manually in their
.bashrcor similar.But for the completion scripts its IMO overkill. We also don't check there whether the shell is really bash or perhaps just POSIX sh.
That being said, if you still insist on keeping the key in the completion scripts, too, just don't pick 1ee68e1.
Cheers,
Chris.