-
Notifications
You must be signed in to change notification settings - Fork 4
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
Fix startup and syntax in python3 #3
Conversation
FWIW, I just verified that the tests pass on the current tip of |
@@ -25,6 +25,9 @@ | |||
from Products.TemporaryFolder.TemporaryFolder import MountedTemporaryFolder | |||
from ZODB.POSException import ConflictError, \ | |||
ReadConflictError, BTreesConflictError | |||
|
|||
from __future__ import print_function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from __future__
must be first line of code in the script (except for module docstring, comments, blank lines etc)
@@ -20,7 +20,7 @@ | |||
import os | |||
import random | |||
import sys | |||
import thread | |||
from six.moves import _thread as thread |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: should be sorted before raw 'import's
values = list(self._data[key].values()) | ||
DEBUG and TLOG('_do_finalize_work: values to notify from ts %s ' | ||
'are %s' % (key, `list(values)`)) | ||
'are %r' % (key, list(values))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
values
is already a list, no need to coerce again.
@@ -17,15 +17,15 @@ | |||
import os | |||
import random | |||
import sys | |||
import thread | |||
from six.moves import _thread as thread |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be sorted before imports
return "id: %s, token: %s, content keys: %s" % ( | ||
self.id, self.token, `self.keys()` | ||
return "id: %s, token: %s, content keys: %r" % ( | ||
self.id, self.token, self.keys() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.keys()
needs to be converted to a list in Python 3, otherwise the repr
contains a dict_keys()
wrapper string
@@ -322,7 +324,7 @@ def _move_item(self, k, current_ts, default=None): | |||
DEBUG and TLOG( | |||
'_move_item: keys for found_ts %s (bucket %s): %s' % ( | |||
found_ts, id(self._data[found_ts]), | |||
`list(self._data[found_ts].keys())`) | |||
repr(list(self._data[found_ts].keys()))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can probably drop the repr
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe by changing the last %s
to %r
in the string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As Tres pointed out: The tests are running fine on master
and should run here, too.
Temporarily allow TravisCI failures for Python 3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now Python 3 creates invalid browser IDs.
t = time() | ||
stamp = TimeStamp(*gmtime(t)[:5]+(t % 60,)) | ||
ts = b2a(stamp.raw()).split(b'=')[:-1][0] | ||
return ts.replace(b'+/', b'-.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method now returns bytes
. This breaks getNewBrowserId
which now created invalid browser ids on Python 3 because they contain the b
of the bytes object. But maybe getNewBrowserId
should return bytes
to fix this.
Fixes BrowserIdManager.py:51: DeprecationWarning: invalid escape sequence \? and similar warnings.
440c517
to
d9a027a
Compare
The Python 3 variant of the "maketrans" logic was faulty (it would not replace "+" by "-" and "/" by ".") causing sporadic test failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I changes the things myself I want to have changed.
Currently Travis CI is allowed to fail the Python 3 tests. They should be fixed before merging this PR.