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

BF: Fixes ioHub compatiblity issues with Python 3. #1770

Merged
merged 2 commits into from Mar 15, 2018

Conversation

Projects
None yet
4 participants
@dvbridges
Contributor

dvbridges commented Mar 14, 2018

The fixes are mainly specific to keyboard devices.

BF: Fixes ioHub compatiblity issues with Python 3.
The fixes are specifically for keyboard devices.
@@ -610,7 +611,7 @@ def createTrialHandlerRecordTable(self, trials, cv_order=None):
for cond_name in self._cv_order:
cond_val = trial[cond_name]
if isinstance(cond_val, basestring):

This comment has been minimized.

@peircej

peircej Mar 14, 2018

Member

If we switch this line then unicode strings passed from Py2 will raise errors. I think you can leave it as basestring but then do thisi import at top: from past.builtins import basestring
see http://python-future.org/compatible_idioms.html

@peircej

Most of these are good (like converting file to open) but some are going to break the Python2.7 compatibility.

@@ -50,7 +50,7 @@ def __init__(self, ioe_array):
@property
def key(self):
return self._key
return str(self._key, 'utf-8')

This comment has been minimized.

@peircej

peircej Mar 14, 2018

Member

This will raise an error on Py2.7 (because can't convert to a bytes string with unicode encoding)
You could do:

from past.builtins import unicode

then

return unicode(self._key, 'utf-8')
@coveralls

This comment has been minimized.

coveralls commented Mar 14, 2018

Coverage Status

Coverage decreased (-0.2%) to 51.598% when pulling 513a6da on dvbridges:ioHub into 22c3bbb on psychopy:master.

@codecov-io

This comment has been minimized.

codecov-io commented Mar 14, 2018

Codecov Report

Merging #1770 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1770   +/-   ##
=======================================
  Coverage   47.09%   47.09%           
=======================================
  Files         218      218           
  Lines       33282    33282           
  Branches     5555     5555           
=======================================
  Hits        15675    15675           
  Misses      16052    16052           
  Partials     1555     1555

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22c3bbb...513a6da. Read the comment docs.

@peircej

This comment has been minimized.

Member

peircej commented on psychopy/iohub/client/__init__.py in 513a6da Mar 14, 2018

This is another one that needs unicode(keys, 'utf-8') and possibly a new import at the top for past.builtins?

This comment has been minimized.

Contributor

dvbridges replied Mar 14, 2018

I kept that as str because it only works if constants.PY3 == True, but should I change that for consistency?

This comment has been minimized.

Member

peircej replied Mar 14, 2018

ah, that wasn't visible to me in the code snippet without expanding the section to include more preceding code. OK, then I don't mind either way

@peircej

This comment has been minimized.

Member

peircej commented on psychopy/iohub/client/__init__.py in 513a6da Mar 14, 2018

d0.find('ERROR') >= 0 seems to be a cumbersome alternative to 'ERROR' in d0 (I know this one wasn't yours David)

@peircej peircej merged commit 2caa8d2 into psychopy:master Mar 15, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgtm analysis: Python No alert changes
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment