Reordering of import statements in the entire phys2bids project #195

eurunuela · 2020-04-02T14:12:01Z

Closes #182.

Proposed Changes

Reorders imports as approved in Discussion on the ordering of library imports #182.

codecov · 2020-04-02T14:16:54Z

Codecov Report

Merging #195 into master will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #195      +/-   ##
==========================================
+ Coverage   94.42%   94.46%   +0.03%     
==========================================
  Files           7        7              
  Lines         574      578       +4     
==========================================
+ Hits          542      546       +4     
  Misses         32       32

Impacted Files	Coverage Δ
phys2bids/interfaces/txt.py	`98.67% <100.00%> (ø)`
phys2bids/phys2bids.py	`90.44% <100.00%> (ø)`
phys2bids/utils.py	`91.54% <100.00%> (ø)`
phys2bids/viz.py	`97.26% <0.00%> (ø)`
phys2bids/physio_obj.py	`91.83% <0.00%> (+0.34%)`	⬆️

smoia · 2020-04-03T08:06:53Z

I know that I took myself out from the review task, but I had a peek this morning at it.
To my understanding, if we want to follow PEP8 conventions as you proposed in #182, the import should be a bit different. libraries such as collections and operator are standard, while numpy is third party.
For instance, taking the txt.py imports, they should follow:

# standard libraries
import logging
from collections import Counter
from operator import itemgetter

# third parties
import numpy as np

# internal
from phys2bids.physio_obj import BlueprintInput

It's quite tricky to categorise "standard libraries", as we use much more often packages like numpy and matplotlib, that are third parties, rather than operator or collections that are standard. I found a list here. I think that another rule of thumb is that if it is in our requirement, then it's a third party. However, it's not easy to say for me either
Maybe @rmarkello could review this PR?

eurunuela · 2020-04-03T08:27:40Z

Good point! I always find it difficult to tell when it's a standard library or a third-party one. To me, they're all third-party 🤣

rmarkello · 2020-04-03T13:30:10Z

Happy to review !

@smoia: that's the right list you linked to, and indeed—if it's in the requirements.txt file then it's third-party (assuming you didn't add any standard library modules to that list 🙈)!

@eurunuela: do you wanna do another re-ordering based on the list @smoia linked describing what modules are standard and then let me know when to take a look at this?

eurunuela · 2020-04-04T08:50:27Z

Sure thing @rmarkello! I’ll let you know when it’s ready.

eurunuela · 2020-04-06T10:04:45Z

@rmarkello forgot to tell you this last commit should've addressed the comments in our previous discussion

rmarkello

Hey @eurunuela ! This looks good; I made a few comments / suggestions that should be pretty straightforward to take care of, but let me know if you have any questions.

Happy to approve once these few minor things are addressed!

phys2bids/interfaces/txt.py

rmarkello · 2020-04-06T15:29:58Z

phys2bids/tests/conftest.py

@@ -1,7 +1,6 @@
 import os
-from urllib.request import urlretrieve
-
 import pytest


pytest is not part of the standard library and should be separate into a third-part imports section.

This point still stands! pytest is not part of the stdlib and should be moved to a third-party library import section.

rmarkello · 2020-04-06T15:30:57Z

phys2bids/tests/test_integration.py

@@ -2,12 +2,13 @@
 import json
 import math
 import os
-from pkg_resources import resource_filename
 import re
 import subprocess



Suggested change

pkg_resources is part of the standard library and should be bundled with the standard imports. It mostly is here, but we just need to get rid of this extra blank line!

This also still stands! pkg_resources shouldn't be separated from standard library imports.

rmarkello · 2020-04-06T15:31:39Z

phys2bids/tests/test_phys2bids.py

 import json
-from phys2bids import phys2bids
+import os
+


Suggested change

Same as the last one; no blank line between the import and from X import Y

Same as above! pkg_resources needs to be part of the standard library imports

phys2bids/tests/test_physio_obj.py

rmarkello · 2020-04-06T15:33:03Z

phys2bids/tests/test_txt.py

@@ -1,5 +1,4 @@
 import math


Suggested change

import math

import math

Need the blank line; pytest is third-party.

Need this blank line. pytest is third-party!

eurunuela · 2020-04-08T07:18:11Z

@rmarkello I'm sorry I made you "waste" your time like that. Apparently my last changes were not correctly pushed (I had just factory reset my computer so something probably did not go as expected with git).

Most of your comments had already been assessed, so this time it should be even easier to review.

Thank you so much! 💯

rmarkello

Still had a few lingering things that needed to be cleaned up; I commented on some "resolved" conversations so hopefully it re-opens them to show you what I'm talking about! Almost there 👍

rmarkello · 2020-04-08T13:34:21Z

phys2bids/tests/conftest.py

@@ -1,7 +1,6 @@
 import os
-from urllib.request import urlretrieve
-
 import pytest


This point still stands! pytest is not part of the stdlib and should be moved to a third-party library import section.

rmarkello · 2020-04-08T13:35:06Z

phys2bids/tests/test_integration.py

@@ -2,12 +2,13 @@
 import json
 import math
 import os
-from pkg_resources import resource_filename
 import re
 import subprocess



This also still stands! pkg_resources shouldn't be separated from standard library imports.

rmarkello · 2020-04-08T13:35:28Z

phys2bids/tests/test_phys2bids.py

 import json
-from phys2bids import phys2bids
+import os
+


Same as above! pkg_resources needs to be part of the standard library imports

rmarkello · 2020-04-08T13:36:02Z

phys2bids/tests/test_txt.py

@@ -1,5 +1,4 @@
 import math


Need this blank line. pytest is third-party!

rmarkello · 2020-04-08T13:39:42Z

Okay, @eurunuela ! That review was a bit weird because I was commenting on issues that had been marked as "resolved" so it's showing them as...still resolved. I unresolved all the still-present issues from my first review—take a look at those (there should be ~4) and let me know what you think!

Thanks so much!

eurunuela · 2020-04-08T14:19:44Z

Okay @rmarkello, I think we're both on the same page now. I'm sorry for the misunderstanding.

Thanks!

rmarkello

LGTM!

eurunuela · 2020-04-08T15:21:46Z

Thanks @rmarkello ! I'll merge it myself. I don't think this issue needs another review (it's a minimal change).

Reorders imports

82852fb

eurunuela requested review from smoia and vinferrer April 2, 2020 14:12

smoia added the Internal Changes affect the internal API. It doesn't increase the version, but produces a changelog label Apr 2, 2020

smoia removed their request for review April 2, 2020 14:18

eurunuela changed the title ~~Reorders imports~~ Reordering of import statements in the entire phys2bids project Apr 2, 2020

smoia requested a review from rmarkello April 3, 2020 07:58

Adresses PR comments

3127d88

rmarkello reviewed Apr 6, 2020

View reviewed changes

Did some minimal reordering

33388c3

rmarkello reviewed Apr 8, 2020

View reviewed changes

Addresses PR comments

da753ad

rmarkello approved these changes Apr 8, 2020

View reviewed changes

eurunuela merged commit 3dc8c8a into physiopy:master Apr 8, 2020

eurunuela deleted the enh/import_ordering branch August 25, 2020 05:20

smoia added the released This issue/pull request has been released. label Oct 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reordering of import statements in the entire phys2bids project #195

Reordering of import statements in the entire phys2bids project #195

eurunuela commented Apr 2, 2020

codecov bot commented Apr 2, 2020 •

edited

Loading

smoia commented Apr 3, 2020

eurunuela commented Apr 3, 2020

rmarkello commented Apr 3, 2020

eurunuela commented Apr 4, 2020

eurunuela commented Apr 6, 2020

rmarkello left a comment

rmarkello Apr 6, 2020

rmarkello Apr 8, 2020

rmarkello Apr 6, 2020

rmarkello Apr 8, 2020

rmarkello Apr 6, 2020

rmarkello Apr 8, 2020

rmarkello Apr 6, 2020

rmarkello Apr 8, 2020

eurunuela commented Apr 8, 2020

rmarkello left a comment

rmarkello Apr 8, 2020

rmarkello Apr 8, 2020

rmarkello Apr 8, 2020

rmarkello Apr 8, 2020

rmarkello commented Apr 8, 2020

eurunuela commented Apr 8, 2020

rmarkello left a comment

eurunuela commented Apr 8, 2020

Reordering of import statements in the entire phys2bids project #195

Reordering of import statements in the entire phys2bids project #195

Conversation

eurunuela commented Apr 2, 2020

Proposed Changes

codecov bot commented Apr 2, 2020 • edited Loading

Codecov Report

smoia commented Apr 3, 2020

eurunuela commented Apr 3, 2020

rmarkello commented Apr 3, 2020

eurunuela commented Apr 4, 2020

eurunuela commented Apr 6, 2020

rmarkello left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eurunuela commented Apr 8, 2020

rmarkello left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmarkello commented Apr 8, 2020

eurunuela commented Apr 8, 2020

rmarkello left a comment

Choose a reason for hiding this comment

eurunuela commented Apr 8, 2020

codecov bot commented Apr 2, 2020 •

edited

Loading