fix: python 2 Function interfaces recompatibility #2093
Conversation
unicode in python 2 functions is a nono
|
@mgxd - this is not a good solution as we do want people to use print function everywhere. it's completely fine in python 2. |
Codecov Report
@@ Coverage Diff @@
## master #2093 +/- ##
==========================================
+ Coverage 72.19% 72.19% +<.01%
==========================================
Files 1158 1160 +2
Lines 58032 58054 +22
Branches 8336 8336
==========================================
+ Hits 41895 41912 +17
- Misses 14815 14820 +5
Partials 1322 1322
Continue to review full report at Codecov.
|
|
@satra hm, wouldn't we want to support all python 2 legacy code though? And then once 1.0 rolls around, drop py2 support altogether |
|
@mgxd - i see a change but no specific test for the issue. could you please add a test? |
|
@mgxd @satra I guess the issue here is that we have the The "right" fix here would not be adding: from __future__ import print_function, division, unicode_literals, absolute_import
from future import standard_library
standard_library.install_aliases()
from builtins import str, bytes, range, zipright after the function definition in the string? |
|
@oesteban To me, tt looks like the function wrapped in the Essentially, this seems to be the chain of events # nipype/misc/utils
from __future__ import unicode_literals # and the others
# function code
import numpy as np
this_array = np.array(['1.1'])
np.savetxt('test.txt', this_array, fmt='%.6f')resulting in Could you expand on what you meant be the "right" fix? |
|
Well, I meant the "right" fix because |
nipype/utils/tests/test_functions.py
Outdated
| assert is_string() == wrapped_func() | ||
|
|
||
| wrapped_func2 = create_function_from_source(getsource(print_statement)) | ||
| wrapped_func2() |
There was a problem hiding this comment.
will raise syntax error if print can't be done - is there a better way to do this?
There was a problem hiding this comment.
with pytest.raises(SyntaxError): wrapped_func2()
There was a problem hiding this comment.
ah but it doesn't error by default. so you should return something from the function call and check with assert.
nipype/utils/tests/test_functions.py
Outdated
| @pytest.mark.skipif(sys.version_info[0] > 2, reason="breaks python 3") | ||
| def test_func_py2(): | ||
| def is_string(): | ||
| return isinstance('string', str) |
There was a problem hiding this comment.
why would this function break python 3?
There was a problem hiding this comment.
perhaps separate out into two tests - the print statement which breaks python 3 and the function which should be fine for both.
There was a problem hiding this comment.
@satra it'll raise an AssertionError - I've isolated the tests though
Fixes #2060 #2038 .
Changes proposed in this pull request
Functionagain