Would you rather deactivate the warning, use explicit encode and decode calls, or are you happy to abandon Python 2 support in this PR?

I would prefer for 3.0.x to retain Python2 support. But we shouldn't invest a lot of effort in such. So I would prefer to:

• Disable newer pylint checks that fail in Python2;
• Focus on modifying dev to conform to recommendations for Python3 for 3.1.0.

couldn't figure out why except Exception as e: throws so many unused-variable warnings about e. Looks like a pylint bug but deactivated these

If "e" is being used, it may be a red herring warning string, with the actual issue being that the variable name does not conform to the specified requirements as it is too short.

Sorry, looks like we might have been doubling up... I'd already pushed changes to fix these errors to dev in 9752aa3 - looks pretty much identical to yours. And yes, I think there is a bug in pylint's handling of those exceptions...

-.-

feel free to close unless we want to make master pylint-compatible before merging dev or cherry-picking 9752aa3.

OK, did a bit of tidying up. I kept most of your changes, especially the explicit use of the io module, which I think allows us to handle locale issues more consistently between python 2 & 3? Otherwise, I've kept a couple of my changes, but nothing major. I'll need to merge this back to dev once we merge this to master...

@maxpietsch, @Lestropie: can I ask you to double-check you're happy with the changes in aa5a354? These are to fix 'unused variable' warnings from my compiler (clang 13.0).

Correct me if I am wrong but I don't think we can follow pylint rules by using io.open(path, 'r', encoding='utf8') and make the code compatible with python 2 and 3.

Python2

with io.open('file', 'r', encoding='utf8') as f: type(f.read())
<type 'unicode'>

Python3

with io.open('file', 'r', encoding='utf8') as f: type(f.read())
<class 'str'>

In configure I used binary mode and encoded/decoded every string manually to make it compatible with Python2 and 3 but that's ugly. So currently, we're breaking Python2 runtime compatibility (not build).

Correct me if I am wrong but I don't think we can follow pylint rules by using io.open(path, 'r', encoding='utf8') and make the code compatible with python 2 and 3.

I've no idea... I don't know whether the type being a regular str in python 2 is necessarily going to cause any issues... I'll give some of these commands a shot under python2 now.

Yep, it's a problem. Just tried to run dwi2response:

dwi2response: [ERROR] Unhandled Python exception:
dwi2response: [ERROR]   TypeError: write() argument 1 must be unicode, not str
dwi2response: [ERROR] Traceback:
dwi2response: [ERROR]   /home/donald/mrtrix3/bin/dwi2response:86 (in execute())
dwi2response: [ERROR]     app.make_scratch_dir()
dwi2response: [ERROR]   /home/donald/mrtrix3/lib/mrtrix3/app.py:310 (in make_scratch_dir())
dwi2response: [ERROR]     outfile.write(WORKING_DIR + '\n')

can I ask you to double-check you're happy with the changes in aa5a354?

Changes in tckedit are not related to fixing unused variable warnings. Others look fine.

I would prefer for 3.0.x to retain Python2 support. But we shouldn't invest a lot of effort in such. So I would prefer to:

• Disable newer pylint checks that fail in Python2;
• Focus on modifying dev to conform to recommendations for Python3 for 3.1.0.

Of the two options

1. reverting the changed open calls and ignoring these warnings just to merge them in again with dev, or
2. abandon python 2 in this PR and tag it appropriately

I initially preferred the latter but as the removal of python 2 is more thoroughly addressed in dev and as the milestone list of the current 3.1.0 is still far from complete and releasing and announcing versions takes time, I'd go with Rob's suggestion. Is everyone happy with that?

OK, I have to admit I was wondering how we were going to maintain python 2/3 compatibility and pass all the pylint tests... I agree it's far simpler just to disable the failing pylint tests on master, and worry only about Python2 compatibility on dev for release in 3.1.0. I'll make the necessary changes in a bit.

Other than that:

Changes in tckedit are not related to fixing unused variable warnings.

They are, in that clang++ version 11 throws this warning:

cmd/tckedit.cpp:173:28: warning: variable 'this_total_count' set but not used [-Wunused-but-set-variable]
    size_t this_count = 0, this_total_count = 0;

Not a problem on CI yet, but I guess it won't be long before that causes failures if we don't address it. And it is true that while this variable gets incremented in the loop, it ends up completely unused. So either we ditch it or we use it - whichever is most appropriate. But this can (and should) all be done in a separate PR since I have a feeling that it doesn't yet cause any failures...

OK, as far as I can tell, these are all the warnings that we currently get on master with pylint 2.11.1:

consider-using-f-string
unspecified-encoding
use-dict-literal
global-variable-not-assigned
consider-iterating-dictionary
consider-using-with
redundant-u-string-prefix
redefined-outer-name
unused-variable
consider-using-in
arguments-renamed

Of these, I reckon we won't be able to fix consider-using-f-string, redundant-u-string-prefix, and unspecified-encoding without breaking Python 2 compatibility. I'll try to make all the changes we can on this branch and disable these 3 warnings on master.

Changes in tckedit are not related to fixing unused variable warnings.

They are, in that clang++ version 11 throws this warning:

Compilation may well throw that warning with some version of the code, but you specifically requested a review of commit aa5a354, and that commit introduces that variable. So that commit in isolation couldn't possibly be responsible for solving that unused variable warning. I'm guessing that you stripped out eh variable altogether in a previous commit, then re-introduced it including not only the definition and increment but also the later utilisation.

Making changes to text file / string data handling / encoding on master does make me a bit nervous. It took a lot of care to have everything working cross-platform and on Python 2 & 3, and it's been accidentally broken on a couple of occasions. That's the primary aspect where I'd prefer to just disable warnings on master and focus on overhauling to Python3 best practise for 3.1.0.

Changes in tckedit are not related to fixing unused variable warnings.

They are, in that clang++ version 11 throws this warning:

Compilation may well throw that warning with some version of the code, but you specifically requested a review of commit aa5a354, and that commit introduces that variable. So that commit in isolation couldn't possibly be responsible for solving that unused variable warning. I'm guessing that you stripped out eh variable altogether in a previous commit, then re-introduced it including not only the definition and increment but also the later utilisation.

No, what I mean is there is an unused variable this_total_count in the code on master - this is what the compiler complains about. My fix did introduce another variable total_count to capture these values and include them in the output header. The alternative would have been to simply remove the this_total_count variable altogether (which arguably would have been the most correct since that wouldn't change the current behaviour at all). But I don't think it's worth worrying about at this stage anyway since it doesn't (yet) cause issues in the CI. We can fix this on dev and leave master alone for now.

Making changes to text file / string data handling / encoding on master does make me a bit nervous. It took a lot of care to have everything working cross-platform and on Python 2 & 3, and it's been accidentally broken on a couple of occasions. That's the primary aspect where I'd prefer to just disable warnings on master and focus on overhauling to Python3 best practise for 3.1.0.

Yes, I think you're right. Having looked at the range of changes we'd have to make to fix all the warnings, even if do ignore the encoding and string handling warnings, it's probably best to just disable all (or at least most) of the failing tests on master for now, as you say. There are one or two minor issues that would be worth fixing though, I'm going through these as we speak. I'll push a more streamlined set of changes when ready.

Closing due to being superseded by #2401.

Still wish to go through the code and remove any hacks that were put in place for Python2 compatibility; #2047 is already part of 3.1.0 milestone.