Migrate codestyle to Black (#267)

* Setup repo for Black and pre-commit - Update setup.py to install developer tools automatically - pre-commit hook to format then lint automatically * Migrate codestyle to Black * [skip ci] Update contributing.md * Fix linting errors in tests. * [skip ci] update flake8 ignore list
pachyderm · May 19, 2021 · 8b5027d · 8b5027d
1 parent 74a9fce
commit 8b5027d
Show file tree

Hide file tree

Showing 36 changed files with 1,190 additions and 512 deletions.
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
@@ -0,0 +1,11 @@
+repos:
+  - repo: https://github.com/psf/black
+    rev: 21.5b0
+    hooks:
+      - id: black
+        language_version: python3
+  - repo: https://gitlab.com/pycqa/flake8
+    rev: 3.9.1
+    hooks:
+      - id: flake8
+        language_version: python3
diff --git a/Makefile b/Makefile
@@ -33,6 +33,9 @@ src/python_pachyderm/proto: docker-build-proto
 
 init:
 	git submodule update --init
+	python -m pip install -U pip wheel setuptools
+	python -m pip install -e .[DEV]
+	pre-commit install
 
 ci-install:
 	sudo apt-get update
@@ -58,7 +61,8 @@ release:
 	twine upload dist/*
 
 lint:
-	flake8 src/python_pachyderm --exclude=src/python_pachyderm/proto --max-line-length=120 --max-doc-length=80
+	black --check --diff .
+	flake8 .
 	PYTHONPATH=./src:$(PYTHONPATH) etc/proto_lint/proto_lint.py
 
 .PHONY: docs docker-build-proto init ci-install ci-setup release lint
diff --git a/contributing.md b/contributing.md
@@ -2,10 +2,17 @@
 
 ## Getting started
 
-When you first clone this repo, make sure to pull submodules as well:
+Setup & initialize virtualenv, for example:
 
 ```bash
-git submodule update --init
+python3 -m venv venv
+source venv/bin/activate
+```
+
+Run the init script which pulls submodules as well as sets up the Python project and install development tools locally:
+
+```bash
+make init
 ```
 
 ## Code
@@ -46,10 +53,10 @@ Code layout, as of 7/2020:
 
 ### Style
 
-We follow PEP-8, with slightly loosened max line lengths. You can check that
-your code changes match the expected style by running `make lint` locally.
-The linter will also run in CI and fail if there are any stylistic
-discrepancies.
+We use `black` as our Python code formatter. After running `make init`,
+there should be a pre-commit hook that runs `black` and `flake8` automatically.
+You can also check that your code changes match the expected style by running `make lint` locally.
+The linter will also run in CI and fail if there are any stylistic discrepancies.
 
 ### Rebuilding protobuf code
 
@@ -79,32 +86,21 @@ test suite, because it tests several variants of python and pachyderm.
 The full test suite takes a long time to run, and will be run anyway in CI, so
 locally it's usually more convenient to run specific tests. To do so:
 
-* Setup & initialize virtualenv
-* Install the dependencies specified in `tox.ini` -- as of 30-7-20, this is:
-    * `pytest==5.3.4`
-    * `pytest-runner==5.2`
-    * `protobuf>=3.11.2`
-    * `grpcio>=1.26.0`
-    * `certifi>=2019.11.28`
+* Setup & initialize virtualenv if you haven't done so already
 * Alternatively, you can use `tox` to create a virtual environment for you:
   ```
   mkdir venvdir
   tox --devenv venvdir -e py38 # one possible environment
   source ./venvdir/bin/activate # activate python environment
   ```
-* Install python-pachyderm into the virtualenv: `pip install -e .`
 * Start the cluster to run on `localhost:30650` -- if the cluster is not
 exposed on `localhost`, you can use `pachctl port-forward` to proxy
 connections.
 * Run the test: `python3 -m pytest tests -k <test name>`
 
 ### Linting
 
-To run the linter locally:
-
-* Setup & initialize virtualenv
-* Install `flake8`
-* Run `make lint`
+To run the linter locally, run `make lint`
 
 ## Rebuilding API docs
 

diff --git a/etc/proto_lint/proto_lint.py b/etc/proto_lint/proto_lint.py
@@ -33,31 +33,33 @@
 
 # Attributes of proto objects that are ignored, because they are built-in from
 # the protobuf compiler
-PROTO_OBJECT_BUILTINS = set([
-    'ByteSize',
-    'Clear',
-    'ClearExtension',
-    'ClearField',
-    'CopyFrom',
-    'DESCRIPTOR',
-    'DiscardUnknownFields',
-    'Extensions',
-    'FindInitializationErrors',
-    'FromString',
-    'HasExtension',
-    'HasField',
-    'IsInitialized',
-    'ListFields',
-    'MergeFrom',
-    'MergeFromString',
-    'ParseFromString',
-    'RegisterExtension',
-    'SerializePartialToString',
-    'SerializeToString',
-    'SetInParent',
-    'UnknownFields',
-    'WhichOneof'
-])
+PROTO_OBJECT_BUILTINS = set(
+    [
+        "ByteSize",
+        "Clear",
+        "ClearExtension",
+        "ClearField",
+        "CopyFrom",
+        "DESCRIPTOR",
+        "DiscardUnknownFields",
+        "Extensions",
+        "FindInitializationErrors",
+        "FromString",
+        "HasExtension",
+        "HasField",
+        "IsInitialized",
+        "ListFields",
+        "MergeFrom",
+        "MergeFromString",
+        "ParseFromString",
+        "RegisterExtension",
+        "SerializePartialToString",
+        "SerializeToString",
+        "SetInParent",
+        "UnknownFields",
+        "WhichOneof",
+    ]
+)
 
 # A list of methods that we do not expect the library to implement
 BLACKLISTED_METHODS = {
@@ -129,7 +131,7 @@
         "list_i_d_p_connectors": ["list_idp_connectors"],
         "create_i_d_p_connector": ["create_idp_connector"],
         "update_i_d_p_connector": ["update_idp_connector"],
-	"get_o_i_d_c_client": ["get_oidc_client"],
+        "get_o_i_d_c_client": ["get_oidc_client"],
         "delete_o_i_d_c_client": ["delete_oidc_client"],
         "list_o_i_d_c_clients": ["list_oidc_clients"],
         "create_o_i_d_c_client": ["create_oidc_client"],
@@ -147,7 +149,7 @@
     },
     Service.VERSION: {
         "get_version": ["get_remote_version"],
-    }
+    },
 }
 
 # Mapping of renamed method arguments. Multiple times of remappings are
@@ -334,17 +336,23 @@
     ],
 }
 
+
 def camel_to_snake(s):
     """Converts CamelCase strings to snake_case"""
-    return s[0].lower() + "".join("_{}".format(c.lower()) if c in UPPERCASE else c for c in s[1:])
+    return s[0].lower() + "".join(
+        "_{}".format(c.lower()) if c in UPPERCASE else c for c in s[1:]
+    )
+
 
 def attrs(obj):
     """Gets the non-private attributes of an object"""
     return [m for m in dir(obj) if not m.startswith("_")]
 
+
 def trim_suffix(s, suffix):
     """Removes a suffix from a string if it exists"""
-    return s[:-len(suffix)] if s.endswith(suffix) else s
+    return s[: -len(suffix)] if s.endswith(suffix) else s
+
 
 def args_set(values):
     s = set()
@@ -358,11 +366,16 @@ def args_set(values):
 
     return s
 
+
 def lint_method(mixin_cls, proto_module, grpc_method_name, mixin_method_name):
     # get the mixin function and its arguments
-    request_cls = getattr(proto_module, "{}Request".format(trim_suffix(grpc_method_name, "Stream")), None)
+    request_cls = getattr(
+        proto_module, "{}Request".format(trim_suffix(grpc_method_name, "Stream")), None
+    )
     mixin_method = getattr(mixin_cls, mixin_method_name)
-    mixin_method_args = set(a for a in inspect.getfullargspec(mixin_method).args if a != "self")
+    mixin_method_args = set(
+        a for a in inspect.getfullargspec(mixin_method).args if a != "self"
+    )
 
     # give a warning for a python function that takes in arguments even
     # though there's no request object for the gRPC function, implying
@@ -373,7 +386,9 @@ def lint_method(mixin_cls, proto_module, grpc_method_name, mixin_method_name):
         return
 
     # find which arguments differ between the python and gRPC implementation
-    request_args = set([n for n in attrs(request_cls) if n not in PROTO_OBJECT_BUILTINS])
+    request_args = set(
+        [n for n in attrs(request_cls) if n not in PROTO_OBJECT_BUILTINS]
+    )
     missing_args = request_args - mixin_method_args
     extra_args = mixin_method_args - request_args
 
@@ -387,6 +402,7 @@ def lint_method(mixin_cls, proto_module, grpc_method_name, mixin_method_name):
     for arg in missing_args - ok_missing_args:
         yield "method {}: missing argument: {}".format(mixin_method_name, arg)
 
+
 def lint_service(service):
     """Lints a given service"""
 
@@ -397,7 +413,9 @@ def lint_service(service):
     grpc_method_names = set(attrs(grpc_cls))
 
     for grpc_method_name in grpc_method_names:
-        if (not grpc_method_name.endswith("Stream")) and "{}Stream".format(grpc_method_name) in grpc_method_names:
+        if (not grpc_method_name.endswith("Stream")) and "{}Stream".format(
+            grpc_method_name
+        ) in grpc_method_names:
             # skip methods with a streaming equivalent, since only the
             # streaming equivalent is implemented
             continue
@@ -410,17 +428,24 @@ def lint_service(service):
             continue
 
         # find if this method is renamed
-        renamed_mixin_method_names = RENAMED_METHODS.get(service, {}).get(mixin_method_name, [mixin_method_name])
+        renamed_mixin_method_names = RENAMED_METHODS.get(service, {}).get(
+            mixin_method_name, [mixin_method_name]
+        )
 
         for mixin_method_name in renamed_mixin_method_names:
             # find if this method isn't implemented
             if mixin_method_name not in mixin_method_names:
-                yield "service {}: missing method: {}".format(service.name, mixin_method_name)
+                yield "service {}: missing method: {}".format(
+                    service.name, mixin_method_name
+                )
                 continue
 
-            for warning in lint_method(mixin_cls, proto_module, grpc_method_name, mixin_method_name):
+            for warning in lint_method(
+                mixin_cls, proto_module, grpc_method_name, mixin_method_name
+            ):
                 yield "service {}: {}".format(service.name, warning)
 
+
 def main():
     warned = False
 
@@ -431,5 +456,6 @@ def main():
 
     sys.exit(1 if warned else 0)
 
+
 if __name__ == "__main__":
     main()
diff --git a/examples/jupyter/merge.py b/examples/jupyter/merge.py
@@ -5,10 +5,11 @@
 
 PRICE = 5
 
+
 def main():
     try:
         weather_filenames = os.listdir("/pfs/weather")
-    except:
+    except FileNotFoundError:
         weather_filenames = []
 
     with open("/pfs/out/data.csv", "w") as out_file:
@@ -19,16 +20,21 @@ def main():
             trip_filepath = "/pfs/trips/{}-{}".format(dt.month, dt.strftime("%d-%y"))
 
             if os.path.exists(trip_filepath):
-                with open("/pfs/weather/{}".format(weather_filename), "r") as weather_file:
+                with open(
+                    "/pfs/weather/{}".format(weather_filename), "r"
+                ) as weather_file:
                     with open(trip_filepath, "r") as trip_file:
                         weather_json = json.load(weather_file)
                         precip = weather_json["daily"]["data"][0]["precipProbability"]
 
                         trip_csv = csv.reader(trip_file)
-                        next(trip_csv) # skip the header row
+                        next(trip_csv)  # skip the header row
                         trips = int(next(trip_csv)[1])
 
-                        writer.writerow([dt.strftime("%Y-%m-%d"), precip, trips, trips * PRICE])
+                        writer.writerow(
+                            [dt.strftime("%Y-%m-%d"), precip, trips, trips * PRICE]
+                        )
+
 
 if __name__ == "__main__":
     main()
diff --git a/examples/opencv/edges/main.py b/examples/opencv/edges/main.py
@@ -2,15 +2,19 @@
 import numpy as np
 from matplotlib import pyplot as plt
 import os
-
+
+
 # make_edges reads an image from /pfs/images and outputs the result of running
 # edge detection on that image to /pfs/out. Note that /pfs/images and
 # /pfs/out are special directories that Pachyderm injects into the container.
 def make_edges(image):
     img = cv2.imread(image)
     tail = os.path.split(image)[1]
-    edges = cv2.Canny(img,100,200)
-    plt.imsave(os.path.join("/pfs/out", os.path.splitext(tail)[0]+'.png'), edges, cmap = 'gray')
+    edges = cv2.Canny(img, 100, 200)
+    plt.imsave(
+        os.path.join("/pfs/out", os.path.splitext(tail)[0] + ".png"), edges, cmap="gray"
+    )
+
 
 # walk /pfs/images and call make_edges on every file found
 for dirpath, dirs, files in os.walk("/pfs/images"):