Python 3.3 #187

Merged
merged 7 commits into from Mar 27, 2013

Projects

None yet

4 participants

@Astrac
Contributor
Astrac commented Mar 24, 2013

This PR contains changes that allow jedi to work on python 3.3. All tests are passing on my local machine and I updated also travis.yml to run CI on version 3.3.

Changes that I had to do:

  • Decoupled the import resolving logic from the imp module defininf a new helper in _compatibilty that choose to use imp or importlib depending on the environment.
  • Changed hash generation for cache to use hashlib rather than the builtin hash function as the latter was generating different hashes for the same string in different executions of the pyhon 3.3 interpreter.
  • Small changes to enhance compatibility.

EDIT: I tried only regression tests, other suites are failing. I'm going to look into it.

Owner

would fix #109

@tkf tkf commented on an outdated diff Mar 24, 2013
jedi/_compatibility.py
@@ -7,11 +7,65 @@
will be dropped, we'll get rid of most code.
"""
import sys
+import imp
+import os
+import io
tkf
tkf Mar 24, 2013 Collaborator

Do you use io here? If not, please remove it. It causes Python 2.5 failure: https://travis-ci.org/davidhalter/jedi/jobs/5755207#L77

@tkf tkf and 1 other commented on an outdated diff Mar 24, 2013
jedi/_compatibility.py
+
+def find_module(string, path=None):
+ """Provides information about a module.
+
+ This function isolates the differences in importing libraries introduced with
+ python 3.3 on; it gets a module name and optionally a path. It will return a
+ tuple containin an open file for the module (if not builtin), the filename
+ or the name of the module if it is a builtin one and a boolean indicating
+ ig the module is contained in a package."""
+ returning = (None, None, None)
+ if is_py33:
+ returning = find_module_py33(string, path)
+ else:
+ returning = find_module_pre_py33(string, path)
+
+ return returning
tkf
tkf Mar 24, 2013 Collaborator

I think it better be:

if is_py33:
    find_module = find_module_py33
else:
    find_module = find_module_pre_py33
find_module.__doc__ = """
....
"""
davidhalter
davidhalter Mar 24, 2013 Owner

I think it's better the way it is :)

tkf
tkf Mar 26, 2013 Collaborator

@Astrac I guess you forget this change :)

@tkf tkf commented on the diff Mar 24, 2013
jedi/cache.py
@@ -312,7 +313,7 @@ def delete_cache(self):
shutil.rmtree(self._cache_directory())
def _get_hashed_path(self, path):
- return self._get_path('%s.pkl' % hash(path))
+ return self._get_path('%s.pkl' % hashlib.md5(path.encode("utf-8")).hexdigest())
tkf
tkf Mar 24, 2013 Collaborator

This is backward incompatible change so we should increase _ModulePickling.version.

@tkf tkf commented on an outdated diff Mar 24, 2013
jedi/parsing_representation.py
@@ -560,7 +560,7 @@ def parent(self):
@parent.setter
def parent(self, value):
self._parent = value
- if self.next:
+ if hasattr(self, "next") and self.next:
tkf
tkf Mar 24, 2013 Collaborator

Let's use EAFP style like this here:

try:
    next = self.next
except AttributeError:
    return
next.parent = value

See common.UncaughtAttributeError for more info.

Contributor
Astrac commented Mar 24, 2013

I fixed some of the things that were pointed in the discussion; I'd like to run tests for python 3.3 on my local box but I don't have a good experience with tox (also my python experience is pretty short) and I'm getting this error:

https://gist.github.com/Astrac/5abdba7db62ac204325e

Am I missing anything really obvious?

Thank you!

Collaborator
tkf commented Mar 24, 2013

tox should work out of the box so it looks strange to me. tox uses virtualenv behind the scene so you can do source .tox/py33/bin/activate in terminal and then just run py.test as usual. If you get the same error then there is a problem with py.test, not tox.

Contributor
Astrac commented Mar 24, 2013

Same issue with activating the virtual environment and using py.test. I also tried creating a new virtual environment and installing tox/pytest in it with the same result. Any other pointer? I'm going to ask for this also elsewhere anyway, thank you for helping!

Collaborator
tkf commented Mar 24, 2013

py.test has some options for debugging plugins such as --traceconfig. Have a look at py.test --help. I think they would be useful.

Contributor
Astrac commented Mar 24, 2013

I attached the additional traceconfig output to the gist, posted a question on SO and and I'm playing with py.test options. Being a python new starter doesn't make it easy, but I'll find out what's wrong :)

Collaborator
tkf commented Mar 24, 2013

How about --debug? It dumps some info to pytestdebug.log.

Contributor
Astrac commented Mar 24, 2013

It doesn't seem to contain anything really special; I just noticed that there are two different confest.py (one in the main jedi folder and one in the test package), could this be related to the reason of the "plugin already registered error"?

I created a gist with the pytestdebug.log content in case more pythonic people can profit from it:

https://gist.github.com/Astrac/b5728dfeb22c0d2fb0f5

Thank you!

EDIT: Sorted, I installed tox in a clean python 2.7 virtual environment and I cleaned up the jedi folder from any pyc file. This finally worked, I'm going to look into failing tests now!

Collaborator
tkf commented Mar 24, 2013

Ah, congrats!

Contributor
Astrac commented Mar 25, 2013

It looks like importlib is falling back to system libraries, whilst imp.find_module is intending the path parameter in an exclusive way ending up raising an exception:

# Using imp find_module
>>> import imp
>>> imp.find_module("abc", ["test/completion/renaming.py"])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib64/python3.3/imp.py", line 220, in find_module
    raise ImportError(_bootstrap._ERR_MSG.format(name), name=name)
ImportError: No module named 'abc'

# Using import find_loader
>>> import importlib
>>> loader = importlib.find_loader("abc", ["test/completion/renaming.py"])
>>> loader.path
'/usr/lib64/python3.3/abc.py'

Does anyone know how to force importlib find_loader to use exclusively a specific path?

Contributor
Astrac commented Mar 25, 2013

Looks like right after asking the question I found a solution, still two failing tests though!

Contributor
Astrac commented Mar 26, 2013

I've one last test to go, but I think that in this case the test is not applicable to python 3.3. In fact the test is in the last lines in std.py:

#62
import threading
#? ['_Verbose', '_VERBOSE']
threading._Verbose

And as you can see in the following change-set the _VERBOSE flag has been removed from the threading module:

http://hg.python.org/releasing/3.3.0/rev/8ec51b2e57c2

Should we make this test run only on version older than python 3.3? Does anyone has a suggestion about how to get this sorted?

Thanks!

Owner

Good enough! Thank you for researching this!

I'm in favor of just removing these four lines. I don't think we're running into troubles there (#62 was most probably addressed in another test anyway).

@tkf tkf and 1 other commented on an outdated diff Mar 26, 2013
jedi/_compatibility.py
is_py25 = sys.hexversion < 0x02060000
+def find_module_py33(string, path=None):
+ returning = (None, None, None)
+ importing = None
+ if path is not None:
+ importing = importlib.machinery.PathFinder.find_module(string, path)
+ else:
+ importing = importlib.machinery.PathFinder.find_module(string, sys.path)
+ if importing is None:
+ importing = importlib.find_loader(string)
tkf
tkf Mar 26, 2013 Collaborator

Probably it is better to have a comment about why we need to fallback to find_loader here. Is it needed to be consistent with imp.find_module?

Astrac
Astrac Mar 27, 2013 Contributor

Yes, the importlib module has a different philosophy in looking for modules and in the way it deals with the sys.path variable. In fact importlib.find_loader is not considering sys.path but sys.meta_path, that has three different importers in it. This specific fallback allows for importing builtin stuffs. I'm commenting these lines so that it will be more clear!

@tkf tkf and 1 other commented on an outdated diff Mar 26, 2013
jedi/_compatibility.py
+def find_module_py33(string, path=None):
+ returning = (None, None, None)
+ importing = None
+ if path is not None:
+ importing = importlib.machinery.PathFinder.find_module(string, path)
+ else:
+ importing = importlib.machinery.PathFinder.find_module(string, sys.path)
+ if importing is None:
+ importing = importlib.find_loader(string)
+
+ if importing is None:
+ raise ImportError
+
+ try:
+ if (importing.is_package(string)):
+ returning = (None, os.path.dirname(importing.path), True)
tkf
tkf Mar 26, 2013 Collaborator

Why returning = everywhere? It looks like you can replace them with simple return.

Astrac
Astrac Mar 27, 2013 Contributor

As a general guideline I ended up for myself, I like designing the code in a way that every function has only one exit point. I see this practice as something similar to "try avoiding goto, continue and break"; in general I think that the flow of execution should be as linear as possible to allow for easier reading. I've to say that my choice of variable names is not really happy in this case though!

@tkf tkf commented on an outdated diff Mar 26, 2013
jedi/_compatibility.py
+ importing = None
+ if path is None:
+ importing = imp.find_module(string)
+ else:
+ importing = imp.find_module(string, path)
+
+ return (importing[0], importing[1], importing[2][2] == imp.PKG_DIRECTORY)
+
+def find_module(string, path=None):
+ """Provides information about a module.
+
+ This function isolates the differences in importing libraries introduced with
+ python 3.3 on; it gets a module name and optionally a path. It will return a
+ tuple containin an open file for the module (if not builtin), the filename
+ or the name of the module if it is a builtin one and a boolean indicating
+ ig the module is contained in a package."""
tkf
tkf Mar 26, 2013 Collaborator

This "ig" looks typo to me.

Collaborator
tkf commented Mar 26, 2013

I think removing the failing test is OK. Probably it is better to reproduce reported bug even without stdlib. Or we can have "tagging" feature for the integration test so that we could exclude this test when using Python 3.3.

Owner

Probably it is better to reproduce reported bug even without stdlib.

Exactly. I normally do that and then add also a failing test for the original problem. That's why I'm not afraid to remove this test.

Contributor
Astrac commented Mar 27, 2013

All tests green! I dropped the incompatible test, I added some comments in that tricky part of find_module_py33 and also changed variable names so that they make a little more sense.

I don't know if to remove the variable assignments in favor of directly returning from the function. I wouldn't see anything wrong in doing it for find_module but I'm a bit concerned about find_module_py33 because of its greater complexity.

Collaborator
tkf commented Mar 27, 2013

I don't know why you think variable assignment reduces complexity. Assigning to a variable and returning it later means you can have any operation on that in between. Returning early means to eliminate this possibility thus reduces complexity. It's not wrong, but I think harder to read.

But @davidhalter will pull the patch if he thinks it's ok. It's just my preference anyway.

Owner

I don't know if to remove the variable assignments in favor of directly returning from the function

I don't see any reason to do something like:

if foo:
    b = 3
else:
    b = 4

return b

instead of using direct returns, so please change it. I don't care about the time you used it for mod_info because there the whole thing is really complex, but in the example above (just one if/else) it's just a waste of lines :)

But @davidhalter will pull the patch if he thinks it's ok. It's just my preference anyway.

I will pull after this one change :)

IMO @Astrac should rebase and squash a few commits. 20 commits for 86 additions and 24 deletions doesn't look nice...

@schlamar schlamar referenced this pull request in srusskih/SublimeJEDI Mar 27, 2013
Closed

Sublime text 3 #18

I got an exception with this patch:

Traceback (most recent call last):
  File "...\jedi\common.py", line 55, in wrapper
    return func(*args, **kwds)
  File "...\jedi\evaluate.py", line 657, in follow_call_list
    result += follow_call(call)
  File "...\jedi\evaluate.py", line 673, in follow_call
    while not s.parent.isinstance(pr.IsScope):
AttributeError: 'NoneType' object has no attribute 'isinstance'

Any ideas?

Owner

@schlamar you dont have it without the patch? And also when does this problem occur?

you dont have it without the patch?

Not sure :)

And also when does this problem occur?

In SublimeJEDI patched with this PR: https://github.com/schlamar/SublimeJEDI/tree/jedi-3.3

Contributor
Astrac commented Mar 27, 2013

The version of jedi you have in that plugin branch is "customized" to work with SublimeJEDI and Sublime 3 and is based on the actual jedi master. It's a very basic integration, it's not fully working and it won't work with this version of jedi. I was thinking to try integrating this version of jedi after we finish with this PR.

@Astrac My SublimeJEDI branch has nothing to do with your SublimeJEDI PR! It is just this jedi version + API update.

Contributor
Astrac commented Mar 27, 2013

@schlamar Oops! Sorry :)

I tried to integrate it too and I found that the import system changed in sublime 3; are you trying to do this as well or the issue is occurring with version 2?

Astrac added some commits Mar 22, 2013
@Astrac Astrac Created find_module helper to handle compatibility with python 3.3
Moved package checking logic in follow_str function

Created find_module compatibility helper method

Conditional implementation of load_module for python 3.3
d481a7a
@Astrac Astrac Fixed follow_definition test de849fb
@Astrac Astrac Fixed caching issues
Fixed exception raised during cache reading

Switched cache hashing to hashlib

In python 3.3 the hash function is returning different hashes during
different executions of the application.
be8ef33
@Astrac Astrac Simplified code for readability
Splitted import compatibility function definition for better readability

Simplified code for python 3.3 load_module implementation
3ef5648
@Astrac Astrac Adding python 3.3 to test environment, mani fixes
Added python 3.3 to test-suite

Removed unused import

Removed unused import

Migrated to EAFP for attribute checking

Bumped version of ModulePickling for migration to hashlib

Added py33 environment to tox

Fixed issue with package importing on python 3.3
07ec134

There are issues not related to Sublime when doing inheritance. Failing code example:

class A(object):
    def test(self):
        print self

class B(A):

    d

Traceback:

>>> source = '\nclass A(object):\n    def test(self):\n        print self\n\nclass B(A):\n\n   d\n'
>>> Script(source, 8, 5, '')
<Script: ''>
>>> _.complete()
Traceback (most recent call last):
  File "jedi\api.py", line 196, in _get_under_cursor_stmt
    stmt = r.module.statements[0]
IndexError: list index out of range

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "jedi\api.py", line 94, in complete
    scopes = list(self._prepare_goto(path, True))
  File "jedi\api.py", line 188, in _prepare_goto
    stmt = self._get_under_cursor_stmt(goto_path)
  File "jedi\api.py", line 198, in _get_under_cursor_stmt
    raise NotFoundError()
jedi.api.NotFoundError

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "jedi\api_classes.py", line 44, in wrapper
    result = func(*args, **kwds)
  File "jedi\api.py", line 100, in complete
    for scope, name_list in scope_generator:
  File "jedi\evaluate.py", line 160, in get_names_of_scope
    non_flow = scope.get_parent_until(pr.Flow, reverse=True)
AttributeError: 'NoneType' object has no attribute 'get_parent_until'
Astrac added some commits Mar 25, 2013
@Astrac Astrac Using PathFinder rather than find_loader to correctly handle paths
Using PathFinder rather than find_loader to correctly handle from ... import ...

Moved away from find_loader in favour of PathFinder also when using sys.path
0b67a08
@Astrac Astrac Making it nicer
Fixed typo in docstring and added some comments in find_module_py33

Removed a test that is not compatible with python 3.3

Better variable names in find_module implementation(s)

Removed variable assignation in favor of direct return statement
124595d
Collaborator
tkf commented Mar 27, 2013

I think the first error @schlamar saw is identical to #162, which means that it exists before this PR.

Contributor
Astrac commented Mar 27, 2013

@schlamar I squashed some commits to make it nicer. I also tried to replicate your issue on dev and I got the same error, so as @tkf says it is not related to this PR.

@Astrac The second error was unknown AFAIS, but you are right, this one exists already in the dev branch. I opened an extra issue for that: #189

Is master more stable than dev? If yes, maybe this PR is better suited to merge in master instead of dev, so there will be a more stable version to use in SublimeJEDI.

Owner

Is master more stable than dev? If yes, maybe this PR is better suited to merge in master instead of dev, so there will be a more stable version to use in SublimeJEDI.

master is totally different than dev (there has been a lot of refactoring ~ 500 commits). There will be a release very soon, so you will probably see it in SublimeJEDI soon.

@davidhalter davidhalter merged commit 3c73a74 into davidhalter:dev Mar 27, 2013

1 check passed

default The Travis build passed
Details
Owner

@Astrac Thanks!!!! great work! hope to see something of you again :) And sorry for all the criticism.

Owner

Argh. lacking stability: https://travis-ci.org/davidhalter/jedi/jobs/5845868

If somebody finds out why this test sometimes fails, let me know.

Contributor
Astrac commented Mar 27, 2013

@davidhalter It has been fun for me to work on this, don't worry! And about the criticism it's fine as well, I wrote my first python less than one month ago and I like to have some feedback and to see how more navigated python developers write code ;)

Tomorrow I'll see if I can look into that intermittently failing test anyway, any idea by anyone is always very appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment