Uh oh!
There was an error while loading. Please reload this page.
First pass at Python2.7 compatibility - #203
Conversation
cguardia
commented
Oct 28, 2019
@chrisrossi@andrewsg This is almost ready, but I'm running into a problem with the kokoro build. Locally, I get 100% test coverage on 2.7, but kokoro fails because it reports only 99%. It does not say which code is not covered. Could someone try this out locally to see if you get the same result? |
cguardia
commented
Oct 29, 2019
@chrisrossi@andrewsg never mind, found out how to fix the coverage issue. This is ready for review. |
| ] | ||
| # If there are any positional arguments, get their names. | ||
| # inspect.signature is not available in Python 2.7, so we use the | ||
| # arguments obtained with inspect.getarspec, which come from the |
There was a problem hiding this comment.
Typo in inspect.getargspec.
| in_options = True | ||
| elif in_options and name != "_options": | ||
| print(slots) |
There was a problem hiding this comment.
Some debugging code that got left in?
| """Less than ordering.""" | ||
| if not isinstance(other, Key): | ||
| return NotImplemented | ||
| raise TypeError |
There was a problem hiding this comment.
I thought NotImplemented was also the protocol for Python 2 comparison functions. Am I mistaken?
There was a problem hiding this comment.
Not sure about the protocol, but it just didn't work. The NotImplemented was being evaluated as an expression and returned a True/False value instead of raising.
| and ``namespace``. | ||
| """ | ||
| return ( | ||
| return ( # pragma: NO COVER |
There was a problem hiding this comment.
Are we unable to test this for some reason?
There was a problem hiding this comment.
_getnewargs is never called in Python 2 code. Might change the NO COVER to NO PY2 COVER.
| "Incomplete Key entry must be last" | ||
| ) | ||
| elif not isinstance(id_, (str, int)): | ||
| elif not isinstance(id_, (str,) + six.integer_types): |
| filters = [] | ||
| match_keys = [] | ||
| for prop in self._model_class._properties.values(): | ||
| # Somehow, Python 2 and 3 return values in diiferent order, sort them. |
There was a problem hiding this comment.
typo: different
Also, is there a production reason to sort them, or is it only to provide consistent ordering in unit tests? If the latter, I'd tend to sort in the test rather than in the code proper.
| traceback = self._exception.__traceback__ | ||
| except AttributeError: # pragma: NO PY3 COVER # pragma: NO BRANCH | ||
| # Python 2 does not have the helpful traceback attribute, and | ||
| # sice the exception is not being handled, it appears that |
| # Python 2 does not have the helpful traceback attribute, and | ||
| # sice the exception is not being handled, it appears that | ||
| # sys.exec_info can't give us the traceback either. | ||
| traceback = None |
There was a problem hiding this comment.
I think old NDB had some way to do this. Peek that code?
There was a problem hiding this comment.
As far a I can see, it just returns a traceback when it was specifically set by set_exception:
https://github.com/GoogleCloudPlatform/datastore-ndb-python/blob/master/ndb/tasklets.py#L331
| query = SomeKind.query().order(SomeKind.foo) | ||
| assert query.map(get_other_foo) == foos | ||
| # TODO: the return with values inside the tasklet crashes tests on Python 2.7 |
There was a problem hiding this comment.
You can just convert these to use "raise Return(x)" instead of "return x". Or if we want to test both, we can run different tests for python 2 and python 3. "@pytest.mark.skipif" is handy for that.
There was a problem hiding this comment.
Turns out it errors out before even running the tests if the return is there, but I'll use the raise Return method.
This is the first pass at getting a 2.7 branch to run. It is not intended for detailed review, but more to give an idea of what needed changing, and to allow evaluation of the main compatibility issues. Note that some of these changes broke some 3.x tests as well, which will be addressed by future work on this.