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

[MRG] remove py2 support #141

Merged
merged 7 commits into from Nov 25, 2018
Merged

Conversation

teonbrooks
Copy link
Member

PR to remove py2 support from mne-bids. mne-python has removed compatibility for py2 and we depend on it. we no longer have mne.externals.six to maintain compatibility.

@teonbrooks
Copy link
Member Author

closes #140

@codecov-io
Copy link

codecov-io commented Nov 22, 2018

Codecov Report

Merging #141 into master will decrease coverage by <.01%.
The diff coverage is 90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #141      +/-   ##
==========================================
- Coverage   94.38%   94.37%   -0.01%     
==========================================
  Files          14       14              
  Lines         979      978       -1     
==========================================
- Hits          924      923       -1     
  Misses         55       55
Impacted Files Coverage Δ
mne_bids/mne_bids.py 94.25% <100%> (ø) ⬆️
mne_bids/datasets.py 70.73% <100%> (ø) ⬆️
mne_bids/tests/test_mne_bids.py 100% <100%> (ø) ⬆️
mne_bids/utils.py 92.7% <86.66%> (-0.04%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 59e52e0...fd5ca10. Read the comment docs.

@jasmainak
Copy link
Member

Can you add a note to the readme that we support only Python 3 now?

@jasmainak jasmainak added this to In progress (max 4) in 0.2 release via automation Nov 23, 2018
@monkeyman192
Copy link
Contributor

monkeyman192 commented Nov 23, 2018

I can't make review comments in the right places, but there are a few places where we should migrate to my py3-thonic code:

except OSError as exc: # Python >2.5

Also,

raise OSError(errno.EEXIST, '"%s" already exists. Please set '

and
raise OSError(errno.EEXIST, '"%s" already exists. Please set '

should raise FileExistsError (and some tests etc will need to be updated to reflect this correctly...)

These are just a few I can think of from a quick scan but there are probably more.
If you think these changes are more appropriate to be in a separate PR I can make one to update some older code for py2 compatibility to nice py3 code...

@sappelhoff
Copy link
Member

sappelhoff commented Nov 23, 2018

If you think these changes are more appropriate to be in a separate PR I can make one to update some older code for py2 compatibility to nice py3 code...

@monkeyman192, if @teonbrooks is fine with it, you can also send a PR to teonbrooks:py2-deletion so that these changes are directly included here.

Also, we should be explicit about which python 3 version, ... 3.6?

@monkeyman192
Copy link
Contributor

I normally spec my code for about 3.4 at the earliest (because I am still running 3.4 on one or two of my machines), however I don't think there is really anything in here that would really change across 3.x versions (other than maybe changing pathing things to path-like objects instead of simply strings which was 3.6 iirc)
It's the weekend now, so I'll look into it on monday :P

@teonbrooks
Copy link
Member Author

@monkeyman192, if @teonbrooks is fine with it, you can also send a PR to teonbrooks:py2-deletion so that these changes are directly included here.

PRs are definitely welcome. I don't know what the more pythonic way would be for those blocks of code you mentioned.

Also, we should be explicit about which python 3 version, ... 3.6?

I agree, we should list a minimum python version. is it 3.4 or 3.6?

@jasmainak
Copy link
Member

Also, we should be explicit about which python 3 version, ... 3.6?

I think we should support all of Python 3 if it's not too much work. It's just nicer for the users if it doesn't come at an extra cost of code complexity. Otherwise, they will have to switch environments just to use mne-bids ...

README.md Outdated
@@ -16,7 +16,8 @@ Installation
------------

We recommend the [Anaconda](https://www.anaconda.com/download/) Python
distribution. Besides `numpy`, `scipy`, `matplotlib`, and `pandas` (which are
distribution. We require that you use a minimum Python version of 3.4.
Copy link
Member

Choose a reason for hiding this comment

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

I would say python 3 ... so we sync with MNE

@@ -3,7 +3,6 @@ node_js:
language: python

env:
- PYTHON_VERSION=2.7
- PYTHON_VERSION=3.6
Copy link
Member Author

Choose a reason for hiding this comment

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

@jasmainak, if we do want all version of Python 3, should we change the environment to a lower version?

Copy link
Member

Choose a reason for hiding this comment

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

you read my mind. I was thinking the same ...

Copy link
Member Author

Choose a reason for hiding this comment

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

it seems that mne is using 3.6 even though it generically states py3. also travis and appveyor yelled about 3.0 not being found. i will be revert back to 3.6

Copy link
Member

Choose a reason for hiding this comment

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

maybe we don't need to be worried about other python versions if mne is not worried ...

0.2 release automation moved this from In progress (max 4) to Needs review Nov 25, 2018
Copy link
Member

@jasmainak jasmainak left a comment

Choose a reason for hiding this comment

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

@teonbrooks you still need to address @monkeyman192 's comments

@teonbrooks
Copy link
Member Author

@teonbrooks you still need to address @monkeyman192 's comments

I did. #141 (comment). It should be a separate pr because making the code more py3 pythonic isn't a blocker for remove py2 support.
I don't know what is to be done since it is @monkeyman192's approach. I can wait for his pr to this branch before this is merged. I tend to like short, focused prs

@jasmainak
Copy link
Member

jasmainak commented Nov 25, 2018

you just need to replace all occurences of errno.EEXIST with FileExists error. I think it's simple enough not to warrant a new PR, but as you like ...

@teonbrooks
Copy link
Member Author

you just need to replace all occurences of errno.EEXIST with FileExists error. I think it's simple enough not to warrant a new PR, but as you like ...

ok, added those. I just assumed that there were more py3ism that were to be implemented so I was just going to save them all. I updated to account for that find/replace.

0.2 release automation moved this from Needs review (max 4) to Reviewer approved Nov 25, 2018
@jasmainak
Copy link
Member

Good to go on my end.

@sappelhoff
Copy link
Member

thanks @teonbrooks! :-)

@sappelhoff sappelhoff merged commit b2d9b1a into mne-tools:master Nov 25, 2018
0.2 release automation moved this from Reviewer approved to Done Nov 25, 2018
@monkeyman192
Copy link
Contributor

monkeyman192 commented Nov 25, 2018

ok, added those. I just assumed that there were more py3ism that were to be implemented so I was just going to save them all. I updated to account for that find/replace.

Oh yeah, my comment was more of a general one about the possibility of updating any code that could be nicer using python 3 syntax/functions.
I'll have a look through the code at some point and see if there are any things that can be made a bit nicer and just make a separate PR if it looks like it is worth changing anything.

Edit: Ok, there were a few little things to fix, nothing major, but I made it a little bit cleaner I think... Should I make a separate PR or something else? (it's only a few small changes...)(cf. monkeyman192@6fdfe2b)

@teonbrooks teonbrooks deleted the py2-deletion branch November 26, 2018 04:33
@teonbrooks
Copy link
Member Author

Edit: Ok, there were a few little things to fix, nothing major, but I made it a little bit cleaner I think... Should I make a separate PR or something else? (it's only a few small changes...)(cf. monkeyman192@6fdfe2b)

@monkeyman192 yes, a separate pr would be great

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
0.2 release
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants