Merge pull request #42 from lsst-sqre/tickets/DM-40638

DM-40638: Switch to Ruff for linting

.pre-commit-config.yaml

@@ -2,15 +2,16 @@ repos:
   - repo: https://github.com/pre-commit/pre-commit-hooks
     rev: v4.4.0
     hooks:
-      - id: trailing-whitespace
-      - id: check-yaml
+      - id: check-merge-conflict
       - id: check-toml
+      - id: check-yaml
+      - id: trailing-whitespace
 
-  - repo: https://github.com/PyCQA/isort
-    rev: 5.12.0
+  - repo: https://github.com/astral-sh/ruff-pre-commit
+    rev: v0.0.286
     hooks:
-      - id: isort
-        additional_dependencies: [toml]
+      - id: ruff
+        args: [--fix, --exit-non-zero-on-fix]
 
   - repo: https://github.com/psf/black
     rev: 23.3.0
@@ -23,15 +24,3 @@ repos:
       - id: blacken-docs
         additional_dependencies: [black==23.3.0]
         args: [-l, '79', -t, py311]
-
-  - repo: https://github.com/pycqa/flake8
-    rev: 6.0.0
-    hooks:
-      - id: flake8
-
-  - repo: https://github.com/pycqa/pydocstyle
-    rev: 6.3.0
-    hooks:
-      - id: pydocstyle
-        additional_dependencies: [toml]
-        files: ^src/

pyproject.toml

@@ -10,7 +10,7 @@ keywords = [
 ]
 # https://pypi.org/classifiers/
 classifiers = [
-    "Development Status :: 4 - Beta",
+    "Development Status :: 5 - Production/Stable",
     "License :: OSI Approved :: MIT License",
     "Programming Language :: Python",
     "Programming Language :: Python :: 3",
@@ -78,27 +78,12 @@ exclude = '''
 # Use single-quoted strings so TOML treats the string like a Python r-string
 # Multi-line strings are implicitly treated by black as regular expressions
 
-[tool.pydocstyle]
-# Reference: https://www.pydocstyle.org/en/stable/error_codes.html
-convention = "numpy"
-add-ignore = [
-    "D105", # Missing docstring in magic method
-    "D104", # Missing docstring in public package
-    "D102", # Missing docstring in public method (needed for docstring inheritance)
-    "D100", # Missing docstring in public module
-    # Properties shouldn't be written in imperative mode. This will be fixed
-    # post 6.1.1, see https://github.com/PyCQA/pydocstyle/pull/546
-    "D401",
-]
-
-[tool.isort]
-profile = "black"
-line_length = 79
-known_first_party = ["rsp_restspawner", "tests"]
-skip = ["docs/conf.py"]
-
 [tool.pytest.ini_options]
 asyncio_mode = "strict"
+filterwarnings = [
+    # Will probably be fixed with JupyterHub v4.
+    "ignore:'pipes' is deprecated:DeprecationWarning:jupyterhub.spawner"
+]
 # The python_files setting is not for test detection (pytest will pick up any
 # test files named *_test.py without this setting) but to enable special
 # assert processing in any non-test supporting files under tests.  We
@@ -123,3 +108,107 @@ strict_equality = true
 warn_redundant_casts = true
 warn_unreachable = true
 warn_unused_ignores = true
+
+# The rule used with Ruff configuration is to disable every lint that has
+# legitimate exceptions that are not dodgy code, rather than cluttering code
+# with noqa markers. This is therefore a reiatively relaxed configuration that
+# errs on the side of disabling legitimate lints.
+#
+# Reference for settings: https://beta.ruff.rs/docs/settings/
+# Reference for rules: https://beta.ruff.rs/docs/rules/
+[tool.ruff]
+exclude = [
+    "docs/**",
+]
+line-length = 79
+ignore = [
+    "ANN101",  # self should not have a type annotation
+    "ANN102",  # cls should not have a type annotation
+    "ANN401",  # sometimes Any is the right type
+    "ARG001",  # unused function arguments are often legitimate
+    "ARG002",  # unused method arguments are often legitimate
+    "ARG005",  # unused lambda arguments are often legitimate
+    "BLE001",  # we want to catch and report Exception in background tasks
+    "C414",    # nested sorted is how you sort by multiple keys with reverse
+    "COM812",  # omitting trailing commas allows black autoreformatting
+    "D102",    # sometimes we use docstring inheritence
+    "D104",    # don't see the point of documenting every package
+    "D105",    # our style doesn't require docstrings for magic methods
+    "D106",    # Pydantic uses a nested Config class that doesn't warrant docs
+    "D205",    # our documentation style allows a folded first line
+    "EM101",   # justification (duplicate string in traceback) is silly
+    "EM102",   # justification (duplicate string in traceback) is silly
+    "FBT003",  # positional booleans are normal for Pydantic field defaults
+    "FIX002",  # point of a TODO comment is that we're not ready to fix it
+    "G004",    # forbidding logging f-strings is appealing, but not our style
+    "RET505",  # disagree that omitting else always makes code more readable
+    "PLR0913", # factory pattern uses constructors with many arguments
+    "PLR2004", # too aggressive about magic values
+    "PLW0603", # yes global is discouraged but if needed, it's needed
+    "S105",    # good idea but too many false positives on non-passwords
+    "S106",    # good idea but too many false positives on non-passwords
+    "S603",    # not going to manually mark every subprocess call as reviewed
+    "S607",    # using PATH is not a security vulnerability
+    "SIM102",  # sometimes the formatting of nested if statements is clearer
+    "SIM117",  # sometimes nested with contexts are clearer
+    "TCH001",  # we decided to not maintain separate TYPE_CHECKING blocks
+    "TCH002",  # we decided to not maintain separate TYPE_CHECKING blocks
+    "TCH003",  # we decided to not maintain separate TYPE_CHECKING blocks
+    "TD003",   # we don't require issues be created for TODOs
+    "TID252",  # if we're going to use relative imports, use them always
+    "TRY003",  # good general advice but lint is way too aggressive
+    "TRY301",  # sometimes raising exceptions inside try is the best flow
+]
+select = ["ALL"]
+target-version = "py310"
+
+[tool.ruff.per-file-ignores]
+"tests/**" = [
+    "C901",    # tests are allowed to be complex, sometimes that's convenient
+    "D101",    # tests don't need docstrings
+    "D103",    # tests don't need docstrings
+    "PLR0915", # tests are allowed to be long, sometimes that's convenient
+    "PT012",   # way too aggressive about limiting pytest.raises blocks
+    "S101",    # tests should use assert
+    "SLF001",  # tests are allowed to access private members
+]
+
+[tool.ruff.isort]
+known-first-party = ["rsp_restspawner", "tests"]
+split-on-trailing-comma = false
+
+[tool.ruff.flake8-bugbear]
+extend-immutable-calls = [
+    "fastapi.Form",
+    "fastapi.Header",
+    "fastapi.Depends",
+    "fastapi.Path",
+    "fastapi.Query",
+]
+
+# These are too useful as attributes or methods to allow the conflict with the
+# built-in to rule out their use.
+[tool.ruff.flake8-builtins]
+builtins-ignorelist = [
+    "all",
+    "any",
+    "dict",
+    "help",
+    "id",
+    "list",
+    "open",
+    "type",
+]
+
+[tool.ruff.flake8-pytest-style]
+fixture-parentheses = false
+mark-parentheses = false
+
+[tool.ruff.pep8-naming]
+classmethod-decorators = [
+    "pydantic.root_validator",
+    "pydantic.validator",
+]
+
+[tool.ruff.pydocstyle]
+convention = "numpy"

src/rsp_restspawner/auth.py

@@ -2,7 +2,7 @@
 
 from __future__ import annotations
 
-from typing import Any, Optional
+from typing import Any
 
 from jupyterhub.app import JupyterHub
 from jupyterhub.auth import Authenticator
@@ -102,7 +102,7 @@ async def authenticate(
 
         This is not used in our authentication scheme.
         """
-        raise NotImplementedError()
+        raise NotImplementedError
 
     def get_handlers(self, app: JupyterHub) -> list[Route]:
         """Register the header-only login and the logout handlers."""
@@ -121,7 +121,7 @@ def login_url(self, base_url: str) -> str:
         return url_path_join(base_url, "gafaelfawr/login")
 
     async def refresh_user(
-        self, user: User, handler: Optional[RequestHandler] = None
+        self, user: User, handler: RequestHandler | None = None
     ) -> bool | AuthInfo:
         """Optionally refresh the user's token."""
         # If running outside of a Tornado handler, we can't refresh the auth

src/rsp_restspawner/exceptions.py

@@ -7,8 +7,6 @@
 
 from __future__ import annotations
 
-from typing import Optional
-
 from httpx import HTTPError, HTTPStatusError, RequestError
 
 __all__ = [
@@ -65,7 +63,7 @@ def from_exception(cls, exc: HTTPError) -> ControllerWebError:
                 body=exc.response.text,
             )
         else:
-            message = f"{type(exc).__name__}: {str(exc)}"
+            message = f"{type(exc).__name__}: {exc!s}"
             if isinstance(exc, RequestError):
                 return cls(
                     message,
@@ -79,11 +77,11 @@ def __init__(
         self,
         message: str,
         *,
-        method: Optional[str] = None,
-        url: Optional[str] = None,
-        status: Optional[int] = None,
-        reason: Optional[str] = None,
-        body: Optional[str] = None,
+        method: str | None = None,
+        url: str | None = None,
+        status: int | None = None,
+        reason: str | None = None,
+        body: str | None = None,
     ) -> None:
         self.message = message
         self.method = method

src/rsp_restspawner/spawner.py

@@ -9,7 +9,7 @@
 from enum import Enum
 from functools import wraps
 from pathlib import Path
-from typing import Any, Concatenate, Optional, ParamSpec, TypeVar
+from typing import Any, Concatenate, ParamSpec, TypeVar
 
 from httpx import AsyncClient, HTTPError, Response
 from httpx_sse import ServerSentEvent, aconnect_sse
@@ -31,7 +31,7 @@
     "RSPRestSpawner",
 ]
 
-_CLIENT: Optional[AsyncClient] = None
+_CLIENT: AsyncClient | None = None
 """Cached global HTTP client so that we can share a connection pool."""
 
 
@@ -195,7 +195,7 @@ def __init__(self, *args: Any, **kwargs: Any) -> None:
         # Holds the future representing a spawn in progress, used by the
         # progress method to know when th spawn is finished and it should
         # exit.
-        self._start_future: Optional[asyncio.Task[str]] = None
+        self._start_future: asyncio.Task[str] | None = None
 
     @property
     def _client(self) -> AsyncClient:
@@ -274,7 +274,7 @@ async def options_form(self, spawner: Spawner) -> str:
         return r.text
 
     @_convert_exception
-    async def poll(self) -> Optional[int]:
+    async def poll(self) -> int | None:
         """Check if the pod is running.
 
         Returns

tests/spawner_test.py

@@ -18,10 +18,7 @@ async def collect_progress(
     spawner: RSPRestSpawner,
 ) -> list[dict[str, int | str]]:
     """Gather progress from a spawner and return it as a list when done."""
-    result = []
-    async for message in spawner.progress():
-        result.append(message)
-    return result
+    return [m async for m in spawner.progress()]
 
 
 @pytest.mark.asyncio

tests/support/controller.py

@@ -4,7 +4,7 @@
 
 import asyncio
 from collections.abc import AsyncIterator
-from datetime import datetime, timedelta
+from datetime import datetime, timedelta, timezone
 
 import respx
 from httpx import AsyncByteStream, Request, Response
@@ -34,7 +34,7 @@ class MockProgress(AsyncByteStream):
     """
 
     def __init__(
-        self, user: str, delay: timedelta, fail_during_spawn: bool = False
+        self, user: str, delay: timedelta, *, fail_during_spawn: bool = False
     ) -> None:
         self._user = user
         self._delay = delay
@@ -50,7 +50,7 @@ async def __aiter__(self) -> AsyncIterator[bytes]:
         # sse-starlette sends these ping events periodically to keep the
         # connection alive. We should just ignore them.
         yield b"event: ping\r\n"
-        yield b"data: " + str(datetime.utcnow()).encode() + b"\r\n"
+        yield b"data: " + str(datetime.now(tz=timezone.utc)).encode() + b"\r\n"
         yield b"\r\n"
 
         yield b"event: info\r\n"
@@ -144,7 +144,9 @@ def events(self, request: Request, user: str) -> Response:
         self._check_authorization(request)
         if not self._lab_status.get(user):
             return Response(status_code=404)
-        stream = MockProgress(user, self.delay, self.fail_during_spawn)
+        stream = MockProgress(
+            user, self.delay, fail_during_spawn=self.fail_during_spawn
+        )
         return Response(
             status_code=200,
             headers={"Content-Type": "text/event-stream"},
@@ -174,7 +176,7 @@ def status(self, request: Request, user: str) -> Response:
         )
 
     def _check_authorization(
-        self, request: Request, admin: bool = False
+        self, request: Request, *, admin: bool = False
     ) -> None:
         authorization = request.headers["Authorization"]
         auth_type, token = authorization.split(None, 1)