-
Notifications
You must be signed in to change notification settings - Fork 280
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
Cleanup some old python 2 idioms #2640
Cleanup some old python 2 idioms #2640
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many files now start with a few empty lines, I left a couple suggestions.
Otherwise, this LGTM.
ee0561e
to
ff843f0
Compare
Oops. I used |
@@ -1,4 +1,4 @@ | |||
from __future__ import print_function | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cphyc were you mentioning empty lines at the start of files because you'd like them to be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, fixing it right now
@@ -13,8 +13,7 @@ def test_cartesian_coordinates(): | |||
# We're going to load up a simple AMR grid and check its volume | |||
# calculations and path length calculations. | |||
ds = fake_amr_ds() | |||
axes = list(set(ds.coordinates.axis_name.values())) | |||
axes.sort() | |||
axes = sorted(set(ds.coordinates.axis_name.values())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 to this change
else: | ||
a.sort(axis=axis) | ||
sorted = a | ||
a_sorted = a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this clearer variable name
…and used builtin sorted function where relevant
Co-authored-by: Corentin Cadiou <contact@cphyc.me>
4f89ee8
to
592a21e
Compare
@yt-fido test this please |
@yt-fido test this please |
object
class is implicit in python 3 so there's no need to keep enforcing it after we dropped support for python 2.The following were done semi-automatically through
2to3
+ manual validation, since in many cases this tool will produce incorrect results (e.g. with sorting numpy arrays)long
(->int
) andunicode
(->str
)next
method instead of__next__
, this change could break some downstream code if the deprecated syntaxself.next()
was used (should be a niche case and is easily fixed)a = sorted(data)
instead ofa=list(data); a.sort()
where relevant (i.e. where it doesn't cause performance hits by adding a copy step)while True
instead ofwhile 1
isinstance(a, mytype)
instead oftype(a) is mytype
. Within tests I conservatively assumed it's preferable to leave the more strict type comparison but replaced some==
comparisons withis
(I don't know if it's even possible to build case where they are not equivalent but the later is definetely correct).