Replace black, isort, and pylint with ruff (#948)

* Make changes based on migration to ruff - remove pylint disable lines - make some changes suggested by ruff - ignore other suggestions
mozilla-it · Aug 1, 2024 · 17ea7d6 · 17ea7d6
1 parent dadc1fa
commit 17ea7d6
Show file tree

Hide file tree

Showing 21 changed files with 209 additions and 400 deletions.
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
@@ -1,14 +1,6 @@
 repos:
-  - repo: local
-    hooks:
-      - id: pylint
-        name: pylint
-        entry: poetry run pylint
-        language: system
-        exclude: ^migrations/
-        types: [python]
   - repo: https://github.com/pre-commit/pre-commit-hooks
-    rev: v2.1.0
+    rev: v4.6.0
     hooks:
       - id: check-added-large-files
       - id: check-json
@@ -23,45 +15,38 @@ repos:
         ]
         exclude: 'k8s/secret.json'
       - id: trailing-whitespace
+
   - repo: local
     hooks:
-      - id: mypy
-        name: mypy
-        entry: poetry run mypy
-        language: system
-        types: [python]
-  - repo: local
-    hooks:
-      - id: bandit
-        name: bandit
-        entry: poetry run bandit
-        args: [-lll, --recursive]
-        language: system
-  - repo: local
-    hooks:
-      - id: detect-secrets
-        name: detect-secrets
-        entry: poetry run detect-secrets-hook
-        args: ['--baseline', '.secrets.baseline']
-        exclude: "poetry.lock"
-        language: system
-  - repo: https://github.com/asottile/seed-isort-config
-    rev: v1.9.3
-    hooks:
-      - id: seed-isort-config
-  - repo: local
-    hooks:
-      - id: isort
-        name: isort
-        entry: poetry run isort
-        args: ["--recursive", "--settings-path", "./pyproject.toml", "."]
-        language: system
-        types: [python]
-  - repo: local
-    hooks:
-      - id: black
-        name: black
-        entry: poetry run black
-        args: ["--config","./pyproject.toml", "."]
-        types: [python]
-        language: system
+    - id: lint
+      name: lint
+      entry: bin/lint.sh
+      args: [lint]
+      language: system
+      types: [python]
+
+    - id: mypy
+      name: mypy
+      entry: bin/lint.sh
+      args: [mypy]
+      language: system
+      types: [python]
+
+    - id: bandit
+      name: bandit
+      entry: bin/lint.sh
+      args: [bandit]
+      language: system
+
+    - id: detect-secrets
+      name: detect-secrets
+      entry: bin/lint.sh
+      args: [detect-secrets]
+      language: system
+
+    - id: format
+      name: format
+      entry: bin/lint.sh
+      args: [format]
+      language: system
+      types: [python]
diff --git a/Makefile b/Makefile
@@ -21,7 +21,7 @@ help:
 	@echo "  build             - build docker containers"
 	@echo "  db-only           - run PostgreSQL server"
 	@echo "  lint              - lint check for code"
-	@echo "  format            - run formatters (black, isort), fix in place"
+	@echo "  format            - run formatter, fix in place"
 	@echo "  setup             - (re)create the database"
 	@echo "  shell             - open a shell in the web container"
 	@echo "  start             - run the API service"
@@ -53,8 +53,7 @@ lint: $(INSTALL_STAMP)
 
 .PHONY: format
 format: $(INSTALL_STAMP)
-	bin/lint.sh black --fix
-	bin/lint.sh isort --fix
+	bin/lint.sh format --fix
 
 .PHONY: db-only
 db-only: .env

diff --git a/bin/lint.sh b/bin/lint.sh
@@ -6,76 +6,76 @@ POETRY_RUN="poetry run"
 CURRENT_DIR=$(CDPATH= cd -- "$(dirname -- "$0")" && pwd)
 BASE_DIR="$(dirname "$CURRENT_DIR")"
 
-bandit () {
-  $POETRY_RUN bandit -lll --recursive "${BASE_DIR}" --exclude "${BASE_DIR}/poetry.lock,${BASE_DIR}/.venv,${BASE_DIR}/.mypy,${BASE_DIR}/build"
-}
-black () {
-  $POETRY_RUN black ${check:+--check} "${BASE_DIR}"
-}
-detect_secrets () {
-  # Scan only files fixed into the repo, omit poetry.lock
-  FILES_TO_SCAN=`git ls-tree --full-tree -r --name-only HEAD | grep -v poetry.lock`
-  $POETRY_RUN detect-secrets-hook $FILES_TO_SCAN --baseline .secrets.baseline
-}
-isort () {
-  $POETRY_RUN isort ${check:+--check-only} "${BASE_DIR}"
-}
-pylint () {
-  $POETRY_RUN pylint "${BASE_DIR}/ctms" "${BASE_DIR}/tests"
-}
-mypy () {
-  $POETRY_RUN mypy "${BASE_DIR}/ctms"
-}
+
+BANDIT_CMD="$POETRY_RUN bandit -lll --recursive ${BASE_DIR} --exclude ${BASE_DIR}/poetry.lock,${BASE_DIR}/.venv,${BASE_DIR}/.mypy,${BASE_DIR}/build"
+
+FORMAT_CMD="$POETRY_RUN ruff format $BASE_DIR"
+
+# Scan only files fixed into the repo, omit poetry.lock
+DETECT_SECRETS_FILES="$(git ls-tree --full-tree -r --name-only HEAD | grep -v poetry.lock)"
+DETECT_SECRETS_CMD="$POETRY_RUN detect-secrets-hook $DETECT_SECRETS_FILES --baseline .secrets.baseline"
+
+LINT_CMD="$POETRY_RUN ruff check $BASE_DIR"
+
+MYPY_CMD="$POETRY_RUN mypy ${BASE_DIR}/ctms"
+
+
 all () {
-  echo "running black"
-  black
-  echo "running isort"
-  isort
+  echo "running format (check only)"
+  $FORMAT_CMD
+  echo "running lint"
+  $LINT_CMD
   echo "running mypy"
-  mypy
-  echo "running pylint"
-  pylint
+  $MYPY_CMD
   echo "running bandit"
-  bandit
+  $BANDIT_CMD
   echo "running detect_secrets"
-  detect_secrets
+  $DETECT_SECRETS_CMD
 }
 
 usage () {
-  echo "Usage: bin/lint.sh [OPTION]"
-  echo " run linting checks"
-  echo "Options":
+  echo "Usage: bin/lint.sh [subcommand] [--fix]"
+  echo " run linting checks, and optionally fix in place (if available)"
+  echo "Subcommand":
   echo "  bandit"
-  echo "  black [--fix]"
   echo "  detect-secrets"
-  echo "  isort [--fix]"
+  echo "  format"
+  echo "  lint"
   echo "  mypy"
-  echo "  pylint"
 }
 
-subcommand='';
-check="true"
-if [ -z $1 ]; then
+if [ -z "$1" ]; then
   all
 else
   subcommand=$1; shift
   case $subcommand in
-    "black" | "isort")
-      case $1 in
-        "--fix")
-          check=""
-        ;;
-      esac
-      case $subcommand in
-        "isort") isort;;
-        "black") black;;
-      esac
-    ;;
-
-    "pylint") pylint;;
-    "mypy") mypy;;
-    "bandit") bandit;;
-    "detect-secrets") detect_secrets;;
-    *) usage;;
+    "format")
+      if [ -n "$1" ] && [ "$1" != "--fix" ]; then
+        usage
+      else
+        check_flag="--check"
+        [ "$1" = "--fix" ] && check_flag=""
+        $FORMAT_CMD ${check_flag:-}
+      fi
+      ;;
+    "lint")
+      if [ -n "$1" ] && [ "$1" != "--fix" ]; then
+        usage
+      else
+        $LINT_CMD ${1:-}
+      fi
+      ;;
+    "mypy")
+      $MYPY_CMD
+      ;;
+    "bandit")
+      $BANDIT_CMD
+      ;;
+    "detect-secrets")
+      $DETECT_SECRETS_CMD
+      ;;
+    *)
+      usage
+      ;;
   esac
 fi
diff --git a/ctms/backport_legacy_waitlists.py b/ctms/backport_legacy_waitlists.py
@@ -8,7 +8,7 @@
 from .schemas import RelayWaitlistInSchema, VpnWaitlistInSchema, WaitlistInSchema
 
 
-def format_legacy_vpn_relay_waitlist_input(
+def format_legacy_vpn_relay_waitlist_input(  # noqa: PLR0912,PLR0915
     db: Session, email_id: UUID4, input_data, schema_class, metrics: Optional[Dict]
 ):
     """
@@ -100,42 +100,38 @@ def format_legacy_vpn_relay_waitlist_input(
                     to_update.append(
                         WaitlistInSchema(name=waitlist.name, subscribed=False)
                     )
-            else:
-                # `relay_waitlist` field was specified.
-                if input_relay_newsletters:
-                    # We are subscribing to a `relay-*-waitlist` newsletter.
-                    # We want to keep the other Relay waitlists already subscribed.
-                    for waitlist in relay_waitlists:
-                        to_update.append(
-                            WaitlistInSchema(
-                                name=waitlist.name,
-                                subscribed=waitlist.subscribed,
-                                fields=waitlist.fields,
-                            )
-                        )
-                    for newsletter in input_relay_newsletters:
-                        name = newsletter["name"].replace("-waitlist", "")
-                        to_update.append(
-                            WaitlistInSchema(
-                                name=name, fields={"geo": parsed_relay.geo}
-                            )
+            elif input_relay_newsletters:
+                # We are subscribing to a `relay-*-waitlist` newsletter.
+                # We want to keep the other Relay waitlists already subscribed.
+                for waitlist in relay_waitlists:
+                    to_update.append(
+                        WaitlistInSchema(
+                            name=waitlist.name,
+                            subscribed=waitlist.subscribed,
+                            fields=waitlist.fields,
                         )
-                elif len(relay_waitlists) == 0:
-                    # We are subscribing to the `relay` waitlist for the first time.
+                    )
+                for newsletter in input_relay_newsletters:
+                    name = newsletter["name"].replace("-waitlist", "")
                     to_update.append(
-                        WaitlistInSchema(name="relay", fields={"geo": parsed_relay.geo})
+                        WaitlistInSchema(name=name, fields={"geo": parsed_relay.geo})
                     )
-                else:
-                    # `relay_waitlist` was specified but without newsletter, hence update geo field
-                    # of all subscribed Relay waitlists.
-                    for waitlist in relay_waitlists:
-                        to_update.append(
-                            WaitlistInSchema(
-                                name=waitlist.name,
-                                source=waitlist.source,
-                                fields={"geo": parsed_relay.geo},
-                            )
+            elif len(relay_waitlists) == 0:
+                # We are subscribing to the `relay` waitlist for the first time.
+                to_update.append(
+                    WaitlistInSchema(name="relay", fields={"geo": parsed_relay.geo})
+                )
+            else:
+                # `relay_waitlist` was specified but without newsletter, hence update geo field
+                # of all subscribed Relay waitlists.
+                for waitlist in relay_waitlists:
+                    to_update.append(
+                        WaitlistInSchema(
+                            name=waitlist.name,
+                            source=waitlist.source,
+                            fields={"geo": parsed_relay.geo},
                         )
+                    )
 
     if to_update:
         formatted["waitlists"] = [wl.model_dump() for wl in to_update]

diff --git a/ctms/bin/client_credentials.py b/ctms/bin/client_credentials.py
@@ -86,7 +86,7 @@ def print_new_credentials(
         )
 
 
-def main(db, settings, test_args=None):
+def main(db, settings, test_args=None):  # noqa: PLR0912
     """
     Process the command line and create or update client credentials
 

diff --git a/ctms/bin/delete_bulk.py b/ctms/bin/delete_bulk.py
@@ -79,4 +79,4 @@ def delete(obj, email, email_file):
 
 
 if __name__ == "__main__":
-    cli()  # pylint: disable=no-value-for-parameter
+    cli()
-Original file line number
+Diff line change
@@ Expand Up / @@ -86,7 +86,7 @@ def print_new_credentials( @@
             )
-    def main(db, settings, test_args=None):
+    def main(db, settings, test_args=None):  # noqa: PLR0912
         """
         Process the command line and create or update client credentials
@@ Expand Down @@
Original file line number	Diff line number	Diff line change
Expand Up		@@ -79,4 +79,4 @@ def delete(obj, email, email_file):


		if __name__ == "__main__":
		cli() # pylint: disable=no-value-for-parameter
		cli()