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

Turn pyenv-mode into a local mode and introduce global-pyenv-mode #30

Closed
wants to merge 1 commit into from
Closed

Conversation

wyuenho
Copy link
Contributor

@wyuenho wyuenho commented Jun 12, 2018

Hi!

I've finally fixed this issue that's been bothering me for a while.

Currently, pyenv-mode is a global minor mode but not the conventional kind setup with define-globalized-minor-mode. It has a global keymap that clobbers re-builder but doesn't offer any help integrating with Projectile satisfactorily.

This PR turns the original pyenv-mode to pyenv-local-mode, the new pyenv-mode will now only turn on pyenv-local-mode and its associated keybindings only on python mode buffers.

In addition, the new global pyenv-mode will integrate with Projectile much more reliably than what's currently written on the README. In addition to trying to match the project name with a pyenv installed python version, it will first look at the project root and use the python version defined in the .python-version file instead.

I've taken care of not breaking backward compatibility in the PR. Ideally I'd like to only prefix all the variables and functions with pyenv instead of pyenv-mode, as per convention, but that's probably a bit too much cos I'll have to deprecate everything...

Hope you like this!

New global `pyenv-mode` will use projectile to set up PYENV_VERSION automatically.
@proofit404
Copy link
Contributor

Hi,

Personally, I don't like that I can't use switch key bindings from dired, shell or compilation modes.

Will projectile-style keymap prefix solve your issue?

If you have .python-version files in your projects you don't need pyenv-mode at all. All commands from the current buffer will have python from pyenv tool.

This mode is useful if you do not have .python-version file in your project.

What do you think?

@wyuenho
Copy link
Contributor Author

wyuenho commented Jun 13, 2018

Personally, I don't like that I can't use switch key bindings from dired, shell or compilation modes.

We can extract the condition in pyenv-local-mode-turn-on to a defcustom, and set the default to a whitelist like '(python-mode dired-mode comint-mode sh-mode term-mode compilation-mode). I'd rather not bind simple commands to some 4 key stroke bindings like projectile's.

If you have .python-version files in your projects you don't need pyenv-mode at all.

Not if you are on OSX, in which the GUI doesn't inherit the shell's environment, and don't want to reinvent all the logic in pyenv-mode to call pythonic-activate.

This mode is useful if you do not have .python-version file in your project.

Also not true, projects like powerline and Spacemacs use pyenv-mode to show the current pyenv version in the mode line. Even if you don't, pyenv's mode line customization is useful to know which pyenv version you are using.

@wyuenho
Copy link
Contributor Author

wyuenho commented Jun 13, 2018

Also, as a result, if you don't look at a .python-version file first, you may activate the wrong virtualenv or not at all if you are just looking at the project name.

@proofit404
Copy link
Contributor

proofit404 commented Jun 13, 2018

Not if you are on OSX, in which the GUI doesn't inherit the shell's environment, and don't want to reinvent all the logic in pyenv-mode to call pythonic-activate.

This already solved by exec-path-from-shell and out of the scope of this package.

Mention this in README is ok.

@proofit404
Copy link
Contributor

I like to make projectile as optional as possible. Second README snippet is ok.

@wyuenho
Copy link
Contributor Author

wyuenho commented Jun 13, 2018

Ok how about this:

pyenv should detect whether a .python-version exists, if it does, do not call pythonic-activate, but simply get the version from the file to update pyenv's mode line info? This way the actual virtualenv's name in .python-version is respected. This is useful for people who share virtualenvs among projects.

I like to make projectile as optional as possible.

It is already optional, see the with-eval-after-load blocks. There's no dependency on projectile, but if it's installed, pyenv just set things up automatically.

@proofit404
Copy link
Contributor

After some time thinking about this I came to this decision:

  1. Everything related to projectile will present in the readme as it is today. Feel free to improve this part of the readme.

  2. If anyone has problems with pyenv-mode keymap, they can defvar mode keymap before require pyenv-mode. They can declare it to nil, to its own keymap or whatever. We can mention this in the readme.

  3. If some major mode keys are hidden by minor-mode keymap there still a local-map which will take precedence before minor-mode map. So everyone can set up local keybindings with major mode hooks. It's ok to mention this trick in the readme.

  4. To show version in the misc info even if there is no version activated by command we can introduce additional global minor pyenv-version-mode. Eval expression in the misc info can check for this variable and show version with $ pyenv local without touching .python-version file in the Emacs Lisp at all.

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants