[READY] Search user defined python binary also in PATH

[vheon]

I'm marking as an RFC because there is also #424 which does almost the same thing.

---

This change is

[Valloric]

Could you summarize the difference between this and #424 for those of us without the full context?

---

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

---

ycmd/utils.py, line 190 [r1] (raw file):
I strongly prefer FindExecutable. Which is a magic name that references the person's prior knowledge of bash, which the code reader not have.

---

Comments from the review on Reviewable.io

[vheon]

Actually this is basically what we've discussed with @allangarret on #424, but I was already experimenting with this approach and I thought that instead than trying out, explaining to him what to do and check I just opened a PR myself.

---

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

---

Comments from the review on Reviewable.io

[vheon]

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

---

ycmd/utils.py, line 190 [r1] (raw file):
Done.

---

Comments from the review on Reviewable.io

[Valloric]

I understand that this PR is a result of a prior discussion, but that's spread out, involves reading the code in the other PR etc. Writing a couple of sentences in this PR describing the intent makes things clearer both for current reviewers and people reaching this PR by going through git history.

---

Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

ycmd/utils.py, line 39 [r2] (raw file):
@micbou Thoughts on the above comment?

---

Comments from the review on Reviewable.io

[vheon]

As of right now is possible to specify which Python interpreter to use when spawning a JediHTTP instance by setting the python_binary_path option, but that option only accept absolute path to a Python binary. With this PR the behaviour is the following:

• If no option is specified the default is sys.executable (as before)
• If a full path is specified, then is used as the Python interpreter to spawn JediHTTP (as before)
• If an executable name is specified then is searched through the user's PATH and the first found is used to spawn a JediHTTP instance

To achieve this I've adapted out FindExecutable to shutil.which

The only doubt that I still have is if we want to check that what is passed is really a Python interpreter, similar to what YCM does before spawning ycmd. Thoughts?

---

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from the review on Reviewable.io

[Valloric]

Thanks for the clarification. I think this approach is good.

I don't think we need to check is it a Python interpreter; we do that in YCM because sys.executable inside vim points to the vim executable, and calling that as Python is bad. For the option, if the user gives us a path to something that's not a Python, it's their fault.

I'm on this, though I would like to see some tests for this. It will require quite a bit of mocking though.

---

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

ycmd/utils.py, line 191 [r2] (raw file):
Do we need this empty line?

---

Comments from the review on Reviewable.io

[micbou]

I tested it in a virtualenv on Windows and it was working so with minor comments.

---

Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

ycmd/utils.py, line 39 [r2] (raw file):
Since it's done in shutil.which, I think we should.

---

ycmd/utils.py, line 188 [r2] (raw file):
This comment should be changed saying that we are backporting which from shutil to Python 2.

---

Comments from the review on Reviewable.io

[allgdante]

I tested on archlinux using virtualenvs for python2 and python3 and the autocompletion works out of the box with third party packages with let g:ycm_python_binary_path = 'python' inside vimrc.

I think we should add a note inside YouCompleteMe documentation about this setting and virtualenv usage, because it will solve, at least, issues 587 and 609.

Please, feel free to close #424 and merge this instead, so on this

---

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Comments from the review on Reviewable.io

[allgdante]

Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

ycmd/utils.py, line 191 [r2] (raw file):
It's not necessary. In fact, in another function inside this source file there is no empty line after the declaration although the function starts with a comment

---

Comments from the review on Reviewable.io

[vheon]

I think we should add a note inside YouCompleteMe documentation about this setting and virtualenv usage, because it will solve, at least, issues 587 and 609.

Once I finish to write the tests and this gets merged in I will send a PR on YCM for updating the docs 👍

---

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

Comments from the review on Reviewable.io

[Valloric]

What's the state of this PR now?

[vheon]

What's the state of this PR now?

I actually forgot about this (shame on me 😭 ). I was on a short vacation, so I'll resume this work ASAP.

[Valloric]

so I'll resume this work ASAP

No need for the ASAP part; take your time. :)

[vheon]

I've updated the jedi_completer tests and added a couple one for FindExecutable.

[coveralls]

Coverage decreased (-0.03%) to 84.927% when pulling 0898d34 on vheon:python-binary-from-path into 18fc8d8 on Valloric:master.

[coveralls]

Coverage decreased (-0.03%) to 84.927% when pulling 90d0ad3 on vheon:python-binary-from-path into 18fc8d8 on Valloric:master.

[coveralls]

Coverage decreased (-0.03%) to 84.927% when pulling 90d0ad3 on vheon:python-binary-from-path into 18fc8d8 on Valloric:master.

[micbou]

Reviewed 4 of 4 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

ycmd/utils.py, line 223 [r4] (raw file):

# to be backward compatible with Python2 and more consistent to our codebase.
def FindExecutable( executable ):

There is still a blank line here. Not important though.

---

ycmd/utils.py, line 246 [r4] (raw file):

      files = [ executable ]
    else:
      files = [ base + ext for ext in WIN_EXECUTABLE_EXTS ]

WIN_EXECUTABLE_EXTS constant does not exist anymore. We need to use the PATHEXT environment variable here. This should fix the AppVeyor runs.

---

Comments from Reviewable

[vheon]

Review status: 4 of 5 files reviewed at latest revision, 3 unresolved discussions.

---

ycmd/utils.py, line 223 [r4] (raw file):

Previously, micbou wrote…

There is still a blank line here. Not important though.

Done.

---

ycmd/utils.py, line 246 [r4] (raw file):

Previously, micbou wrote…

WIN_EXECUTABLE_EXTS constant does not exist anymore. We need to use the PATHEXT environment variable here. This should fix the AppVeyor runs.

Done.

---

Comments from Reviewable

[coveralls]

Coverage decreased (-0.05%) to 84.903% when pulling b7ab0d2 on vheon:python-binary-from-path into 18fc8d8 on Valloric:master.

[vheon]

Anything else to do here? the coverage decreased because more Windows code was added and coveralls counts only the linux build (maybe would be actually helpful to look at codecov.io... I guess I'll do that this weekend)

[micbou]

Tests are not passing on Windows.

[coveralls]

Coverage decreased (-0.03%) to 84.924% when pulling 75e7002 on vheon:python-binary-from-path into 18fc8d8 on Valloric:master.

[vheon]

@vheon Did you read my messages on gitter? I see that you are struggling to make it work on Windows. Don't bother and use my changes. I can also send my changes against your PR if you want.

No I didn't saw it, but I guess that I'm close enough to at least see if I can make it work 😝

[coveralls]

Coverage decreased (-0.1%) to 84.849% when pulling d9ce74d on vheon:python-binary-from-path into 47304e1 on Valloric:master.

[coveralls]

Coverage decreased (-0.1%) to 84.849% when pulling 521492f on vheon:python-binary-from-path into 47304e1 on Valloric:master.

[coveralls]

Coverage decreased (-0.1%) to 84.854% when pulling abb7e3e on vheon:python-binary-from-path into 47304e1 on Valloric:master.

[coveralls]

Coverage decreased (-0.2%) to 84.736% when pulling cccfd83 on vheon:python-binary-from-path into 47304e1 on Valloric:master.

[coveralls]

Coverage decreased (-0.2%) to 84.736% when pulling 275b13b on vheon:python-binary-from-path into 47304e1 on Valloric:master.

[coveralls]

Coverage decreased (-0.2%) to 84.736% when pulling cef8799 on vheon:python-binary-from-path into 47304e1 on Valloric:master.

[vheon]

Now I think I got it. In the end I made what @micbou already did, but oh well 😝 For the coverage drop I'm looking at codecov, since the coverage went down because more windows-only code was needed for windows 😒

[micbou]

Do you want to wait for codecov to merge this PR?

Previously, vheon (Andrea Cedraro) wrote…

Now I think I got it. In the end I made what @micbou already did, but oh well 😝 For the coverage drop I'm looking at codecov, since the coverage went down because more windows-only code was needed for windows 😒

---

Reviewed 1 of 1 files at r7, 1 of 5 files at r14, 1 of 3 files at r15, 2 of 2 files at r16.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

ycmd/utils.py, line 222 [r16] (raw file):

  for exe in _GetPossibleWindowsExecutable( filename ):
    if os.path.exists( exe ) and not os.path.isdir( exe ):

These two conditions can be replaced by os.path.isfile( exe ).

---

ycmd/utils.py, line 235 [r16] (raw file):

  # Directories usually have execute permission so we need to exclude them.
  if ( not os.path.isdir( filename )

This can also be replaced by os.path.isfile( filename ). In fact, it is better because it checks for existence too.

---

ycmd/completers/typescript/typescript_completer.py, line 102 [r16] (raw file):

      _logger.error( BINARY_NOT_FOUND_MESSAGE )
      raise RuntimeError( BINARY_NOT_FOUND_MESSAGE )
    _logger.info( 'found tsserver at {0}'.format( binarypath ) )

Capital letter?

---

ycmd/tests/utils_test.py, line 479 [r16] (raw file):

  with TemporaryExecutable() as executable:
    dirname, exename = os.path.split( executable )
    relative_executable = os.path.join( '.', exename )

Doesn't matter much but '.' can be replaced by os.curdir.

---

ycmd/tests/utils_test.py, line 484 [r16] (raw file):

@patch.dict( 'os.environ', { 'PATH': tempfile.gettempdir() } )

Those patch.dict are nice.

---

ycmd/tests/utils_test.py, line 491 [r16] (raw file):

def FindExecutable_ReturnNone_IfFileIsNotExecutable_test():

The underscore between ReturnNone and IfFileIsNotExecutable seems superfluous.

---

Comments from Reviewable

[vheon]

It would actually be nice to have codecov right away but since it simplify the use of Jedi for virtualenv I'm not sure 😕 What do you think?

Previously, micbou wrote…

Do you want to wait for codecov to merge this PR?

---

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

ycmd/utils.py, line 222 [r16] (raw file):

Previously, micbou wrote…

These two conditions can be replaced by os.path.isfile( exe ).

Done.

---

ycmd/utils.py, line 235 [r16] (raw file):

Previously, micbou wrote…

This can also be replaced by os.path.isfile( filename ). In fact, it is better because it checks for existence too.

Done.

---

ycmd/completers/typescript/typescript_completer.py, line 102 [r16] (raw file):

Previously, micbou wrote…

Capital letter?

Done.

---

ycmd/tests/utils_test.py, line 479 [r16] (raw file):

Previously, micbou wrote…

Doesn't matter much but '.' can be replaced by os.curdir.

I actually prefer this form since it seems clear (at least to me) that I want to pass something like `./executable`.

---

ycmd/tests/utils_test.py, line 491 [r16] (raw file):

Previously, micbou wrote…

The underscore between ReturnNone and IfFileIsNotExecutable seems superfluous.

Done.

---

Comments from Reviewable

[coveralls]

Coverage decreased (-0.2%) to 84.736% when pulling 4268395 on vheon:python-binary-from-path into e00b7ce on Valloric:master.

[micbou]

Let's merge this. Adding coverage for Windows is less important.

@homu r+

Previously, coveralls wrote…

Coverage decreased (-0.2%) to 84.736% when pulling 4268395 on vheon:python-binary-from-path into e00b7ce on Valloric:master.

---

Reviewed 3 of 3 files at r17.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

ycmd/tests/utils_test.py, line 479 [r16] (raw file):

Previously, vheon (Andrea Cedraro) wrote…

I actually prefer this form since it seems clear (at least to me) that I want to pass something like ./executable.

Agreed.

---

Comments from Reviewable

[homu]

📌 Commit 4268395 has been approved by micbou

[homu]

⌛ Testing commit 4268395 with merge ec3c0f1...

[homu]

☀️ Test successful - status

[micbou]

We need to document this feature in YCM README.