search

Powerlevel9k / powerlevel9k

Powerlevel9k was a tool for building a beautiful and highly functional CLI, customized for you. P9k had a substantial impact on CLI UX, and its legacy is now continued by P10k.
https://github.com/romkatv/powerlevel10k
MIT License
13.46k stars 948 forks source link

[Bugfix] pyenv weirdness #1283

Closed Syphdias closed 5 years ago

Syphdias commented 5 years ago

I was a bit confused to find __p9k_$1_prompt_segment here...

This produced the wrong segment content, e.g. systemblack. Also resolves #1280 by not displaying the version if it is "system" and P9K_PYENV_PROMPT_ALWAYS_SHOW=false

Syphdias commented 5 years ago

Question: Why not use pyenv global?

laggardkernel commented 5 years ago

@Syphdias Your simplification slows down the virtual environment detection dramatically. Get python version from PYENV_VERSION is much faster than pyenv version-name. The similar thing happens with rbenv and nodenv.

❯ time (pyenv version-name)
2.7.16
0.05s user 0.04s system 96% cpu 0.097 total

❯ time (echo $PYENV_VERSION)
2.7.16
0.00s user 0.00s system 59% cpu 0.001 total

pyenv version-name is just an implementation of the version detection logic:

  1. PYENV_VERSION
  2. .python-version
  3. ${PYENV_ROOT}/version

There's some overhead of pyenv version-name compared with checking PYENV_VERSION directly.

Syphdias commented 5 years ago

There's some overhead of pyenv version-name compared with checking PYENV_VERSION directly.

The expected behaviour is, that the segment spits out the same output as the command to check the version. Otherwise, false information is provided by the segment that might confuse a user. To get exactly that behaviour you need every logic handled by pyenv version-name. How do we get this exact behaviour now?

  1. Use the (slow) command itself
  2. reimplement every logic, including future changes

If you choose 2 you can improve performance if there is unnecessary overhead. It's not true that performance improvements cannot be done with 1. You could utilize techniques like caching or prefetching to get the output faster.

The old implementation did not reimplement all the logic that is used by pyenv version-name thus proving false output. That's why I decided to remove the shortcut. Of course, you could go with the current version as a compromise of speed and wrong output.

laggardkernel commented 5 years ago

@Syphdias Obviously, I'm not that insane to implement every logic again by myself. I'm only indicating you should check PYENV_VERSION first, then fallback to pyenv version-name.

Syphdias commented 5 years ago

you should check PYENV_VERSION first

As mentioned in #1281 PYENV_VERSION cannot be trusted. And it is a compromise. There are pros and cons to trusting PYENV_VERSION anyways. I'm not a user. I cannot decide which one is the better version. The current version just has less unexpected behaviour.

Syphdias commented 5 years ago

Closing this PR since #1285 makes this one redundant.

@laggardkernel, I'll leave it to @bhilburn which one he thinks is best for users, since the current version does have its drawbacks as well.

laggardkernel commented 5 years ago

Sorry to bother you. I didn't realize there was another related pr being merged. It seems my words is too late.