Skip to content

Clean up a few Python 2-isms #586

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

Merged
merged 6 commits into from
Sep 23, 2020
Merged

Clean up a few Python 2-isms #586

merged 6 commits into from
Sep 23, 2020

Conversation

rahulporuri
Copy link
Contributor

@rahulporuri rahulporuri commented Aug 21, 2020

This PR removes a few remaining Python 2-isms from the codebase, namely :

  • Updating use of super i.e. instead of super(ClassName, self), we just use super(),
  • Remove coding cookies from the top of files because on Python 3, utf-8 is the default source code file encoding,
  • Remove u-prefix strings because on Python 3 they don't do anything,
  • Remove an if/elif/else conditional which was always true for python 3.

Note to reviewer : Each commit in this PR makes one and only one kind of change (listed above) so reviewing this PR commit-by-commit will be straightforward.

TODO:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/tutorial.rst
  • Changes documented in docs/release.rst
  • AppVeyor and Travis CI passes
  • Test coverage is 100% (Coveralls passes)

@pep8speaks
Copy link

pep8speaks commented Aug 21, 2020

Hello @rahulporuri! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-09-18 19:27:16 UTC

@clbarnes
Copy link
Contributor

Could you also remove the references to python <3 in the requirements files? I was going to make my own PR but it should just go here. Patch file was

From 4510eb512e7011e80f14dbdb08fe869931364ebf Mon Sep 17 00:00:00 2001
From: Chris Barnes <[email protected]>
Date: Fri, 11 Sep 2020 17:54:04 +0100
Subject: [PATCH] Remove python<3.0 references from requirements

---
 requirements_dev_minimal.txt  | 4 +---
 requirements_dev_numpy.txt    | 4 +---
 requirements_dev_optional.txt | 2 +-
 3 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/requirements_dev_minimal.txt b/requirements_dev_minimal.txt
index 9aefb92..7928558 100644
--- a/requirements_dev_minimal.txt
+++ b/requirements_dev_minimal.txt
@@ -5,6 +5,4 @@ numcodecs==0.6.4
 msgpack-python==0.5.6
 setuptools-scm==3.3.3
 # test requirements
-pytest==5.2.0; python_version > '3.0'
-# don't let pyup change this, needed until we drop support for py27
-pytest==4.6.5; python_version < '3.0' # pyup: ignore
+pytest==5.2.0
diff --git a/requirements_dev_numpy.txt b/requirements_dev_numpy.txt
index b5d87c4..f2ac012 100644
--- a/requirements_dev_numpy.txt
+++ b/requirements_dev_numpy.txt
@@ -1,6 +1,4 @@
 # Break this out into a separate file to allow testing against
 # different versions of numpy. This file should pin to the latest
 # numpy version.
-numpy==1.17.2; python_version > '3.0'
-# don't let pyup change this, needed until we drop support for py27
-numpy==1.16.4; python_version < '3.0' # pyup: ignore
+numpy==1.17.2
diff --git a/requirements_dev_optional.txt b/requirements_dev_optional.txt
index 5777ad5..95a9ee5 100644
--- a/requirements_dev_optional.txt
+++ b/requirements_dev_optional.txt
@@ -18,4 +18,4 @@ pytest-cov==2.7.1
 pytest-doctestplus==0.4.0
 pytest-remotedata==0.3.2
 h5py==2.10.0
-s3fs==0.3.4; python_version > '3.0'
+s3fs==0.3.4
-- 
2.28.0

@rahulporuri
Copy link
Contributor Author

rahulporuri commented Sep 12, 2020

@clbarnes I've addressed your comment about the requirements files.

Sai Rahul Poruri added 5 commits September 18, 2020 09:52
	modified:   zarr/indexing.py
	modified:   zarr/n5.py
	modified:   zarr/storage.py
	modified:   zarr/tests/test_convenience.py
utf-8 is the default source code file encoding on python 3
the string prefixes dont do anything on python 3 and are visual noise

	modified:   zarr/tests/test_core.py
	modified:   zarr/tests/test_creation.py
	modified:   zarr/tests/test_filters.py
	modified:   zarr/tests/test_hierarchy.py
	modified:   zarr/util.py
The conditional always evaluates to True on Python 3 so the rest of the
if/elif/else conditional has been removed

	modified:   zarr/storage.py
	modified:   docs/release.rst
@Carreau
Copy link
Contributor

Carreau commented Sep 18, 2020

Rebased to fix conflicts.

remove the import entirely and use os.replace instead of replace

	modified:   zarr/storage.py
@Carreau
Copy link
Contributor

Carreau commented Sep 18, 2020

I believe the coverage failure is due the the decrease in number of LOC infiles that are not 100% covered.

As the total number of lines decrease, if the number of uncovered lines decrease we get a relative change of percentage, leading to failure, even if no actual code get un-covered.

+1

@rahulporuri
Copy link
Contributor Author

Thanks @Carreau for the fixes. I noticed in the middle of the week that there were conflicts and I was hoping to make time to address them this weekend :D

@Carreau
Copy link
Contributor

Carreau commented Sep 19, 2020

I noticed in the middle of the week that there were conflicts and I was hoping to make time to address them this weekend :D

No worries, I was on other things so it was relatively straightforward. Still like to get other more senior core dev approval on this.

@alimanfoo alimanfoo added this to the v2.5 milestone Sep 23, 2020
@jakirkham jakirkham merged commit e3cdd1a into zarr-developers:master Sep 23, 2020
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.

6 participants