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

Various fixes for Python 2 compatibility #33

Closed
wants to merge 1 commit into from

Conversation

lrq3000
Copy link

@lrq3000 lrq3000 commented Jan 27, 2018

This PR fixes Python 2 compatibility.

It is dependent on this Py2 compatibility fix in ipyvolume: widgetti/ipyvolume#103

Also I'm not sure how to fix the following line elegantly:

https://github.com/nipy/niwidgets/compare/master...lrq3000:py2fix?expand=1#diff-c4e3c3c90dfe314c2eaad3135f7992d4L46

But in any case, in Python 2, os.stat() and thus os.file.exists() expect a str, not a Path object. Maybe we can do a branching depending on Python 2 or Python 3?

@dgutman
Copy link

dgutman commented Jan 30, 2018

I am trying to pull this PR to test locally... I am having the python2.7 compatibility issues...

I tried some variants, but wasn't sure of the syntax to try and test this..
git fetch origin pull/33/lrq3000:py2fix

@lrq3000
Copy link
Author

lrq3000 commented Feb 1, 2018

Yes this is normal, it's because widgetti/ipyvolume#103 needs also to be pulled in ipyvolume, else it won't work.

However, even if with this PR and the one for ipyvolume the niwidgets are indeed displayed with no Python compatibility error, I could not display correctly my maps, I just get a pinkish colored volume (but maybe that was because of my mask). But this would be the purpose of another PR I guess, so I leave this issue out for now.

@janfreyberg
Copy link
Contributor

Hi there,

thanks for submitting this PR. I'm actually planning to remove the reliance on pathlib entirely, since much of nibabel doesn't play well with pathlib. Hopefully this will fix these issues.

In the long run, I'm not sure how much I will invest into supporting python 2. Is there a particular reason you're still using it, such as a neuroimaging package that doesn't support python 3?

@lrq3000
Copy link
Author

lrq3000 commented Apr 23, 2018

@janfreyberg Yes I keep on using Python2 merely because some packages still do not play well with Python3 but I think most packages are migrating now so it should be fine if you move to support Python3 only :-)

@janfreyberg
Copy link
Contributor

Cool, thanks for the feedback. I'll try to not put any obviously breaking python 3 only features in anyways.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants