-
-
Notifications
You must be signed in to change notification settings - Fork 423
Fix zsh shell hook performance issue #456
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
Conversation
The precmd hook is causing a significant overhead on every command executed in the shell. The shell hook does not need to be this strict however, it can also run only on directory changes, ensuring that the shell remains snappy while executing commands. Also contains a similar fix for fish. Fixes #259.
Is checking for What do you think about the (second part of the) workaround outlined in #259 (comment)? |
As a zsh poweruser I can safely state that the changing venv is a way too infrequent edge-case to sacrifice shell performance for. In case you'd still run into that, you can cd out from the dir and back again. People seem to be OK with not handling that edge-case but re-gaining shell performance. TBQH I'm not a fish user, so I might remove the fish fix and let someone with fish experience fix that one. Gonna look into the --no-rehash solution. |
That's actually the sole job of |
Currently, the shell cmd
So by not rechecking after an invocation, you single-handedly break this entire command. |
@native-api interesting, because when I use |
I suggest tracing. I'm also very curious. |
The current hook is rather dumb -- it just activates the selected environment blindly. AFAICS, the most productive way, instead of trying to dodge executing it altogether and hope no-one notices, would be to make it more intelligent and not do any unnecessary work. |
I would really dislike the behavior as implemented for fish shell. It seems very unintuitive. Maybe let's settle for just the zsh behavior for this PR. I wouldn't mind championing this as a different issue. |
Should be separate PR to deal with fish perf issue
Removed the fish parts. Gonna spend some additional time to figure out how to best deal with the zsh precmd use-case. I checked yesterday and for me the hook in general doesn't seem to set VIRTUALENV_DIR and other env vars (even without these changes). Maybe it's set up incorrectly for me. (Still have to understand a bit more what exactly the env vars bring to the table compared to the pyenv shims). |
We aren't merging anything that breaks things. Or we'll be swamped in bug reports that things no longer work and will have to fix that pronto in addition to reputational damages. |
Do you have any intention of fixing the highest voted issue on your project? |
PRs can be reopened. And it seems to me that feedback is exactly what I gave in the closing message. I closed it rather than just request changes because the way it is now, it looks like it has to be completely different to actually fix the issue -- so there's little point in keeping it in this topic, you'll have to start a new branch from scratch anyway. |
...So I honestly don't see what the big deal is here. Sorry if I came as haughty/condescending/annoyed, I had no intention to insult anyone. |
The precmd hook is causing a significant overhead on every command executed in the shell.
The shell hook does not need to be this strict however, it can also run only on directory changes, ensuring that the shell remains snappy while executing commands.
Also contains a similar fix for fish.
Fixes #259.