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

PR: Send Spyder python path to introspection plugins #4962

Merged
merged 12 commits into from
Aug 31, 2017

Conversation

rlaverde
Copy link
Member

@rlaverde rlaverde commented Aug 14, 2017

Fixes: #4410

Supersedes #4915
Undo some changes of #4263

At the moment the spyder path is only used by jedi

@rlaverde rlaverde added this to the v3.2.2 milestone Aug 14, 2017
@rlaverde rlaverde self-assigned this Aug 14, 2017
@rlaverde rlaverde requested a review from ccordoba12 August 14, 2017 23:31
@pep8speaks
Copy link

pep8speaks commented Aug 14, 2017

Hello @rlaverde! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on August 30, 2017 at 00:09 Hours UTC

@rlaverde rlaverde force-pushed the introspection-python-path branch from 3efe697 to b23d8aa Compare August 15, 2017 00:06
@@ -30,6 +30,8 @@
except ImportError:
jedi = None

JEDI_010 = [int(i) for i in jedi.__version__.split('.')] >= [0, 10, 0]
Copy link
Member

Choose a reason for hiding this comment

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

We have programs.is_module_installed exactly for this :-)

@@ -110,7 +111,7 @@ def main_window(request):
# (it's faster and less memory consuming not to use it!)
marker = request.node.get_marker('use_introspection')
if marker:
os.environ['SPY_TEST_USE_INTROSPECTION'] = 'True'
os.environ['SPY_TEST_USE_INTROSPECTION'] = 'USE_INTROSPECTION'
Copy link
Member

Choose a reason for hiding this comment

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

Please change this to True. That could fix the failing test below.

@ccordoba12
Copy link
Member

The way to implement this for Rope is the following:

  1. The project attribute of RopePlugin has a prefs sub-attribute, as can be seen here:

    https://github.com/spyder-ide/spyder/blob/3.x/spyder/utils/introspection/rope_plugin.py#L74

  2. You can set a property in prefs called python_path, which is a list of additional paths to perform code completion. I think the way to do this is

     self.project.prefs.set('python_path', ['path1', 'path2'])
    

    but you need to test it to see if it's right.

  3. The previous step needs to be done every time we call the get_completions method of RopePlugin.

@@ -110,7 +111,7 @@ def main_window(request):
# (it's faster and less memory consuming not to use it!)
marker = request.node.get_marker('use_introspection')
if marker:
os.environ['SPY_TEST_USE_INTROSPECTION'] = 'True'
os.environ['SPY_TEST_USE_INTROSPECTION'] = True
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be 'True', not True.

@rlaverde rlaverde force-pushed the introspection-python-path branch from 85dafe8 to 1f3cd95 Compare August 23, 2017 15:30
@ccordoba12
Copy link
Member

@rlaverde, please remove @dalthviz test because is timing out. Instead, please add tests to test_rope_plugin.py and test_jedi_plugin.py that get completions of some test module placed in a temp directory.

@rlaverde
Copy link
Member Author

please remove @dalthviz test because is timing out. Instead, please add tests to test_rope_plugin.py and test_jedi_plugin.py that get completions of some test module placed in a temp directory.

I was trying to fix it without success 😞 I'll also try to move it to an editor test, maybe loading the main window is what's causing it to timeout.

@rlaverde rlaverde force-pushed the introspection-python-path branch 4 times, most recently from 9164e61 to a3ef289 Compare August 25, 2017 14:20
@ccordoba12
Copy link
Member

@rlaverde, I think that adding the test to test_rope_plugin.py and test_jedi_plugin.py is easier and simpler than testing completion in our widgets.

@rlaverde rlaverde force-pushed the introspection-python-path branch from dad09dc to 7698e01 Compare August 25, 2017 17:27
@rlaverde
Copy link
Member Author

rlaverde commented Aug 25, 2017

I think that adding the test to test_rope_plugin.py and test_jedi_plugin.py is easier and simpler than testing completion in our widgets.

Yes, I'll give a last try to the integration test and I'll do that (the test is running pretty well in my local configuration, I don't know way CI is failing 😞 )

@andfoy
Copy link
Member

andfoy commented Aug 25, 2017

@rlaverde Have you tried to debug those tests via SSH on Travis (https://docs.travis-ci.com/user/running-build-in-debug-mode/) and RDP on AppVeyor (https://www.appveyor.com/docs/how-to/rdp-to-build-worker/)?

@rlaverde rlaverde force-pushed the introspection-python-path branch from 7698e01 to fc03f98 Compare August 25, 2017 20:45
@rlaverde rlaverde force-pushed the introspection-python-path branch from fc03f98 to 9e775dc Compare August 25, 2017 22:21
@rlaverde rlaverde force-pushed the introspection-python-path branch from 9e775dc to aa9ac51 Compare August 25, 2017 22:24
@rlaverde rlaverde requested a review from ccordoba12 August 28, 2017 17:45
editor.introspector.plugin_manager.close()


@pytest.mark.skipif(True, reason="This test fails too much in the CI :(")
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove this condition and change the one below to

not JEDI_010 or os.environ.get('CI', False)

so we can run this test locally.

@ccordoba12
Copy link
Member

@dalthviz, @arman00 and @laikh, please help us to test this PR to see if it fixes the problem with code completion in the Editor when using projects.

Thanks for your help!

@ccordoba12 ccordoba12 changed the title PR: Send spyder path to Introspection plugins PR: Send Spyder python path to introspection plugins Aug 28, 2017
@dalthviz
Copy link
Member

@ccordoba12 It works for me 👍 🎉 , I tested opening and closing projects and changing the PYTHONPATH.

@arman00
Copy link

arman00 commented Aug 29, 2017

Unfortunately it doesn't work for me. After I cloned from github , I switched to the 3.x branch. Version is 3.2.2.dev0.

It works if I create a new project, but with my existing project which has different modules in different folders, it doesn't work. I add those folders into the PYTHONPATH.

@dalthviz
Copy link
Member

Hi @arman00 to test this you need to checkout the @rlaverde branch introspection-python-path, did you do that?

@arman00
Copy link

arman00 commented Aug 29, 2017

Oh I see... Didn't know that :)
I will do it as soon as I have the chance again
Thanks

@dalthviz
Copy link
Member

Just in case, to test this you can clone the fork of @rlaverde and then checkout the corresponding branch.

@goanpeca
Copy link
Member

goanpeca commented Aug 29, 2017

You can do

$ git clone https://github.com/rlaverde/spyder.git
$ cd spyder
$ git checkout introspection-python-path
$ python bootstrap.py

@rlaverde rlaverde force-pushed the introspection-python-path branch from 53dfcec to 25a5c3b Compare August 30, 2017 00:09
@laikh
Copy link

laikh commented Aug 30, 2017

Hi guys,

Finally managed to test your solution and am happy to report that this fixed the issue for me!! 👍
I tested it with version 3.2.0 as 3.2.1 seems to be still not available for anaconda.

Thanx a lot, looking forward to the next version!

sincerely,
Michael

@arman00
Copy link

arman00 commented Aug 30, 2017

Yes it works for me, too!!
Thanks for the instructions to test this!

@ccordoba12
Copy link
Member

Great news!! Thanks a lot to you for helping us to test this :-)

@ccordoba12 ccordoba12 merged commit e5f2faa into spyder-ide:3.x Aug 31, 2017
ccordoba12 added a commit that referenced this pull request Aug 31, 2017
@rlaverde rlaverde deleted the introspection-python-path branch August 31, 2017 11:43
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.

8 participants