improve python 3 compatibility when parameterize_dirs is False

[wtriplett]

Tested in python 3.5 and 2.7.12 before and after fix. Used non-specific/empty encode() based on what looks like the common usage elsewhere in code.

Fixes #1762

[oesteban]

LGTM - thanks @wtriplett

[codecov-io]

Current coverage is 72.44% (diff: 81.81%)

Merging #1764 into master will increase coverage by <.01%

@@             master      #1764   diff @@
==========================================
  Files          1055       1055          
  Lines         52823      52844    +21   
  Methods           0          0          
  Messages          0          0          
  Branches       7672       7672          
==========================================
+ Hits          38266      38284    +18   
- Misses        13345      13347     +2   
- Partials       1212       1213     +1   

Powered by Codecov. Last update 2d4726f...cff71a1

[satra]

@wtriplett - could you please add #1762 as a test case? essentially something like the following which should fail before your fix is applied:

def test_unicode_parameterization():
    wf = some workflow
    wf.config['execution']['parameterize_dirs'] = True
    error_not_raised = True
    try:
       wf.run()
    except UnicodeError:
        error_not_raised = False
    assert error_not_raised

[wtriplett]

@satra: Ok, I can do that.

[wtriplett]

@satra: So I'm not sure how the automated tests will end up working with this new test case. I guess it might be easier just to call _parameterization_dir() with input string > 32 characters, but that seemed a little too specific of a test.

Anyway, hope this helps.

[wtriplett]

The CircleCI tests are also running here without the fix (e060bbc) applied. The expected results would be pass on python2.7 and fail on 3.5.

https://circleci.com/gh/wtriplett/nipype/2

[satra]

make this a local change rather than a global change.

[satra]

local change for wf: wf.config['execution']['parameterize_dirs'] = False

[satra]

these two statements are not required for a workflow since you set wf.base_dir = wd later.

[satra]

shouldn't this be specifically a TypeError?

[wtriplett]

Good question. I was looking at the other tests, and they all seemed to catch generic exceptions. It dawned on me that if this is a test of a configuration specific execution pathway, that maybe it was better to catch a general exception in case there was another reason for failure and the assertion would still work rather than bailing out.

[satra]

in this case since we know it's a TypeError we should use that. the intent of a Test should be to catch the most specific error, because the other types of errors may require different handling.

for many of the tests in this file, they do raise a generic exception, but this should also be improved ( ping @djarecka )

[wtriplett]

Roger that - I'll revert back to TypeError.

[djarecka]

@satra : I don't understand this try/except block, since we expect wr.run() to work, and if we get ANY exception, the test should fail (and pytest will inform what exception has been raised). IMO, we should remove try/except block at all, just keep wr.run(). The same is true about all test that have assert not error_raised. And the test that have assert error_raised can be rewritten using with pytest.raises(Exception).

If you agree with me, I can clean whole file after this PR is merged (unless you prefer to do it before).

[satra]

let's do it after it's merged. but yes - we should clean up logic where we can.