Skip to content
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

mrtrix3.image.statistic(): Fix parameter naming #1504

Merged
merged 6 commits into from
Nov 27, 2018

Conversation

Lestropie
Copy link
Member

app.error() call contained a reference to the function being invoked, rather than the parameter that was passed to the function.

Not hugely consequential given the script is throwing an error at that point anyway; but nevertheless preferable for a more appropriate error message than "TypeError: must be str, not function".

Observed on forum.

app.error() call contained a reference to the function being invoked, rather than the parameter that was passed to the function.
@Lestropie Lestropie self-assigned this Nov 25, 2018
@Lestropie Lestropie requested a review from a team November 25, 2018 09:23
jdtournier
jdtournier previously approved these changes Nov 26, 2018
@jdtournier
Copy link
Member

jdtournier commented Nov 26, 2018

Same issue here as on #1505, but with added goodness:

## testing "lib/mrtrix3/_5ttgen/gif.py"...
 [ ERROR ]
************* Module mrtrix3._5ttgen.gif
lib/mrtrix3/_5ttgen/gif.py:21:2: W0611: Unused import os (unused-import)
lib/mrtrix3/_5ttgen/gif.py:21:2: W0611: Unused import os (unused-import)
lib/mrtrix3/_5ttgen/gif.py:28:2: W0611: Unused import os (unused-import)
lib/mrtrix3/_5ttgen/gif.py:29:2: W0611: Unused app imported from mrtrix3 (unused-import)
lib/mrtrix3/_5ttgen/gif.py:28:2: W0611: Unused import os (unused-import)

Given that this is all based on master, all parent commits passed testing, and none of the commits since have touched this file, I'm stumped. The only explanation as far as I can tell is that pylint has suddenly decided to become stricter...

In any case, I'll try to fix this one too.

@jdtournier
Copy link
Member

I note a stack of errors in the TravisCI logs at the pip3 install pylint stage. Not sure whether the version of pylint that is eventually installed is the right one. I note pylint 2.2 just came out - tests run clean locally on my system if I use the previous 2.1.1 version, but not 2.2 (on top of that, the tests for labelsgmfix cause a frank crash of pylint...). I've just added a line to run_pylint to output the version information so we can see what is running exactly (though I note I'll need to fix that, it's not quite right...).

@jdtournier
Copy link
Member

OK, this looks like it works...

@Lestropie: please review these changes and make sure you're happy. There's changes to your GIF algorithm, and to the run_pylint script - both of which are yours. Since this is going to master, I think it's best you give it the thumbs-up (I can't add you as a reviewer since you submitted the PR originally...).

jdtournier
jdtournier previously approved these changes Nov 26, 2018
@Lestropie
Copy link
Member Author

The only explanation as far as I can tell is that pylint has suddenly decided to become stricter

On cursory inspection perhaps the new pylint emits unused-import instead of unused-variable, the latter of which was explicitly silenced on the relevant lines (which is acceptable but should only be used where absolutely necessary).

There's changes to your GIF algorithm ...

Not mine.

@Lestropie Lestropie requested a review from a team November 26, 2018 22:23
Copy link
Contributor

@thijsdhollander thijsdhollander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, looks like this is fixed then!

@Lestropie Lestropie merged commit a93b432 into master Nov 27, 2018
@Lestropie Lestropie deleted the python_image_statistic_fix branch November 27, 2018 01:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants