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

s:init_python: handle g:jedi#force_py_version again #834

Merged
merged 1 commit into from
Jul 4, 2018

Conversation

blueyed
Copy link
Collaborator

@blueyed blueyed commented Jun 23, 2018

It was removed previously, since it now refers to the environment being
used.

This brings it back for now.

There could be a separate var for it later, and/or it needs changes
later (or rather the environment needs a new variable name, since it
should refer to the executable really) - but this is good for now I think.

Fixes #833

@blueyed
Copy link
Collaborator Author

blueyed commented Jun 23, 2018

@davidhalter
What do you think about having the old behavior (only) with g:jedi#force_py_version, and adding something like g:jedi#use_python (and b:jedi_use_python) for specifying the environment then?

The tricky part currently is that you cannot differentiate between the Python to load Jedi and the Python that Jedi uses.

I think having used force_py_version for the new environments was wrong.

If you agree, the following should get applied here, too (+doc):

diff --git i/autoload/jedi.vim w/autoload/jedi.vim
index d039e8a..3c1a229 100644
--- i/autoload/jedi.vim
+++ w/autoload/jedi.vim
@@ -70,9 +70,9 @@ function! s:init_python() abort
           throw 'jedi-vim requires Vim with support for Python 2 or 3.'
       endif
     else
-       if g:jedi#force_py_version =~# '^3'
+       if g:jedi#force_py_version ==# '3'
            let py_loader_version = 3
-        elseif g:jedi#force_py_version =~# '^2'
+        elseif g:jedi#force_py_version ==# '2'
             let py_loader_version = 2
         else
             throw printf("jedi-vim: could not determine Python version from '%s' (%s)",

@davidhalter
Copy link
Owner

IMO I wouldn't do that. Python 2 is dying soon and I'm probably not going to support it beyond 2020. People should switch. If they still want Python 2, they can just use an older version of everything without bug fixes and stuff.

This PR goes into the same direction, we're making changes, because someone is having issues with Python 3.2. IMO this is just too much maintenance work for little gain. It seems to be working very well with most users and there has been one case where it didn't, but that's really just one case.

So as I said, I'm kind of against it, but if you really want this we can maybe just use a new variable force_base_python that we don't really mention loudly in docs. Your call.

@blueyed
Copy link
Collaborator Author

blueyed commented Jun 23, 2018

I get your point, but I think it's not too hard to support this.

Re g:jedi#force_py_version: do you want to keep using it for the envs then?
But there it is going to be a executable instead of a number (#836) - although a number could also be used to match against the env's sys_info then.

@davidhalter
Copy link
Owner

Re g:jedi#force_py_version: do you want to keep using it for the envs then?

Yes. I think most people just want to specify a Python version and not an environment. I think the environment should be a separate setting if people really want that.

@blueyed
Copy link
Collaborator Author

blueyed commented Jun 25, 2018

Hmm.
So a new setting for the base Python should be used then (this PR).

g:jedi#force_py_version could then support both versions (could be looked up from the envs), or an explicit env (using its executable). I find the name a bit misleading though, so that might have been a good chance to clean this up.

@davidhalter
Copy link
Owner

davidhalter commented Jun 25, 2018

The reason why I wouldn't change the name is that people are already using it in configs. What would be substantially better? use_python_version? It's pretty similar after all.

I agree however that it's not the best name.

@davidhalter
Copy link
Owner

If you changed something that was related to my comment, please rebase, the "old" logos are still in this diff.

It was removed previously, since it now refers to the environment being
used.

This brings it back for now.

There could be a separate var for it later, and/or it needs changes
later (or rather the environment needs a new variable name, since it
should refer to the executable really) - but this is good for now I think.

Fixes davidhalter#833
@blueyed
Copy link
Collaborator Author

blueyed commented Jul 2, 2018

This allows for g:jedi#loader_py_version to override g:jedi#force_py_version, but does not document it.

@blueyed blueyed requested a review from davidhalter July 2, 2018 12:04
@davidhalter
Copy link
Owner

I have quickly reviewed it and it looks generally good. I generally like the naming.

The point is just, I'm really against re-introducing this. Apart from one guy that runs Python 3.2 (and that's extremely old) - nobody has complained about the lack of this feature. This guy could also just be using Jedi < 0.10.0 with an older jedi-vim version, which should still work.

Since I don't fell like I should just decide this, I'll let you. You did the work and it's not a big deal to maintain it (probably). If you don't agree with me - that's fine, we will then just merge it. It's generally a very small thing and we should probably use our time for discussions on other things :)

@blueyed blueyed merged commit 0361d6c into davidhalter:master Jul 4, 2018
@blueyed blueyed deleted the init-force branch July 4, 2018 15:55
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