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

MacApp: add site-packages to jedi configuration #12271

Closed
wants to merge 3 commits into from

Conversation

mrclary
Copy link
Contributor

@mrclary mrclary commented Apr 10, 2020

Description of Changes

  • Wrote at least one-line docstrings (for any new functions)
  • Added unit test(s) covering the changes (if testable)
  • Included a screenshot or animation (if affecting the UI, see Licecap)

When launching Spyder from non-conda environments, jedi does not add site-packages for target conda environments properly (see davidhalter/jedi#1540).
I've added the site-package for the target environment to the extra_paths configuration for jedi.

Issue(s) Resolved

Fixes #12259

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct:
@mrclary

@ccordoba12
Copy link
Member

Great work finding the problem for this one @mrclary!

Please rebase this and all your other pending PRs so you can get the fixes to our tests.

@mrclary mrclary force-pushed the pyenv-jedi branch 2 times, most recently from d08fe17 to 75e8a83 Compare April 11, 2020 03:58
spyder/utils/programs.py Outdated Show resolved Hide resolved
@pep8speaks
Copy link

pep8speaks commented Apr 16, 2020

Hello @mrclary! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 639:17: E129 visually indented line with same indent as next logical line

Line 146:13: E129 visually indented line with same indent as next logical line

Comment last updated at 2020-06-25 15:30:35 UTC

@goanpeca
Copy link
Member

Hi @mrclary thanks for working on this. After 4.1.3 is release we are working to get all these PRs merged so we can start working also on installers on our side ;-)

Sorry for the lack of response in the past weeks. Cheers!

@goanpeca
Copy link
Member

goanpeca commented May 8, 2020

@mrclary is there any testing we could add for these changes?

@@ -138,6 +139,19 @@ def alter_subprocess_kwargs_by_platform(**kwargs):
# We "or" them together
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this `alter_subprocess_kwargs_by_platform``

We could add a test to check that the expected values on windows are there etc.

@@ -967,6 +981,20 @@ def check_python_help(filename):
return False


def get_python_site_packages(pyexec):
"""Get the site-packages for a given python executable"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docstrings are sentences so let's add the period :-)

"""Get the site-packages for a given python executable."""

Copy link
Member

@goanpeca goanpeca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good @mrclary

My suggestion is writing tests to check all the expected changes are in place and also add some future proofing.

Thanks!

@mrclary
Copy link
Contributor Author

mrclary commented May 8, 2020

@goanpeca , thanks for reviewing the code and I'll be happy to address your suggestions. However, before I do, let me ask how 'future proof' do we want to be? This PR is just a kluge for a bug in jedi (davidhalter/jedi#1540) which is already fixed. I did not expect the changes in this PR to be permanent, but to be removed as soon as python-language-server adopts the next latest version of jedi (>0.17.0)

@goanpeca
Copy link
Member

goanpeca commented May 8, 2020

@mrclary so, anything that is temporal please let's add a TODO: or FIXME: tags. Also please open issue to keep track of the fix that we need to revert once jedi is updated?

For the rest that is not patch work on jedi, let's add tests :-)

@mrclary
Copy link
Contributor Author

mrclary commented May 8, 2020

@mrclary so, anything that is temporal please let's add a TODO: or FIXME: tags. Also please open issue to keep track of the fix that we need to revert once jedi is updated?

For the rest that is not patch work on jedi, let's add tests :-)

Got it. I really don't see any use for get_python_site_packages outside of this kluge, so I will forgo tests for that.

@mrclary
Copy link
Contributor Author

mrclary commented May 9, 2020

I think this is ready

goanpeca
goanpeca previously approved these changes May 9, 2020
Copy link
Member

@goanpeca goanpeca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work @mrclary !

@goanpeca
Copy link
Member

goanpeca commented May 9, 2020

I think this is ready @ccordoba12, should we create the 4.1.x branch and merge this to 4.x ?

@mrclary
Copy link
Contributor Author

mrclary commented Aug 21, 2020

This PR is obsolete with the advent of Jedi 0.17.1

@mrclary mrclary closed this Aug 21, 2020
@mrclary mrclary deleted the pyenv-jedi branch August 29, 2020 01:42
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.

MacApp: Jedi Completion Not Entirely Working from Non-conda environment
4 participants