Convert to Ruff and fix most issues it uncovered

Switch to the Annotated syntax for FastAPI handlers so that there
is no need to whitelist the FastAPI dependency functions. Exclude
the diagnostic of os.getenv in configuration for now, since I will
convert to pydantic-settings in an upcoming PR.

.pre-commit-config.yaml

@@ -2,22 +2,14 @@ repos:
   - repo: https://github.com/pre-commit/pre-commit-hooks
     rev: v4.5.0
     hooks:
-      - id: check-yaml
+      - id: check-merge-conflict
       - id: check-toml
+      - id: check-yaml
+      - id: trailing-whitespace
 
-  - repo: https://github.com/PyCQA/isort
-    rev: 5.13.2
-    hooks:
-      - id: isort
-        additional_dependencies:
-          - toml
-
-  - repo: https://github.com/psf/black
-    rev: 24.2.0
-    hooks:
-      - id: black
-
-  - repo: https://github.com/PyCQA/flake8
-    rev: 7.0.0
+  - repo: https://github.com/astral-sh/ruff-pre-commit
+    rev: v0.2.1
     hooks:
-      - id: flake8
+      - id: ruff
+        args: [--fix, --exit-non-zero-on-fix]
+      - id: ruff-format

changelog.d/20240216_150548_rra_DM_42937.md

@@ -0,0 +1,3 @@
+### Other changes
+
+- Use [Ruff](https://docs.astral.sh/ruff/) for linting and formatting instead of Black, flake8, and isort.

pyproject.toml

@@ -60,30 +60,6 @@ exclude_lines = [
     "if TYPE_CHECKING:"
 ]
 
-[tool.black]
-line-length = 79
-target-version = ["py311"]
-exclude = '''
-/(
-    \.eggs
-  | \.git
-  | \.mypy_cache
-  | \.tox
-  | \.venv
-  | _build
-  | build
-  | dist
-)/
-'''
-# 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.isort]
-profile = "black"
-line_length = 79
-known_first_party = ["vocutouts", "tests"]
-skip = ["docs/conf.py"]
-
 [tool.mypy]
 disallow_untyped_defs = true
 disallow_incomplete_defs = true
@@ -118,6 +94,129 @@ asyncio_mode = "strict"
 # listed in python_files.
 python_files = ["tests/*.py", "tests/*/*.py"]
 
+# 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
+target-version = "py312"
+
+[tool.ruff.lint]
+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
+    "PLR0911", # often many returns is clearer and simpler style
+    "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
+    "S107",    # 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
+    "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
+
+    # The following settings should be disabled when using ruff format
+    # per https://docs.astral.sh/ruff/formatter/#conflicting-lint-rules
+    "W191",
+    "E111",
+    "E114",
+    "E117",
+    "D206",
+    "D300",
+    "Q000",
+    "Q001",
+    "Q002",
+    "Q003",
+    "COM812",
+    "COM819",
+    "ISC001",
+    "ISC002",
+
+    # TEMPORARY
+    "RUF009",
+]
+select = ["ALL"]
+
+[tool.ruff.lint.per-file-ignores]
+"src/uws/handlers.py" = [
+    "D103",    # FastAPI handlers should not have docstrings
+]
+"src/vocutouts/config.py" = [
+    "S108",    # use of /tmp is safe in this context
+]
+"src/vocutouts/handlers/**" = [
+    "D103",    # FastAPI handlers should not have docstrings
+]
+"src/vocutouts/workers.py" = [
+    "S108",    # use of /tmp is safe in this context
+]
+"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
+    "S106",    # tests are allowed to hard-code dummy passwords
+    "SLF001",  # tests are allowed to access private members
+]
+
+[tool.ruff.lint.isort]
+known-first-party = ["vocutouts", "tests"]
+split-on-trailing-comma = false
+
+# These are too useful as attributes or methods to allow the conflict with the
+# built-in to rule out their use.
+[tool.ruff.lint.flake8-builtins]
+builtins-ignorelist = [
+    "all",
+    "any",
+    "help",
+    "id",
+    "list",
+    "type",
+]
+
+[tool.ruff.lint.flake8-pytest-style]
+fixture-parentheses = false
+mark-parentheses = false
+
+[tool.ruff.lint.pydocstyle]
+convention = "numpy"
+
 [tool.scriv]
 categories = [
     "Backwards-incompatible changes",

src/vocutouts/actors.py

@@ -101,7 +101,7 @@ def cutout(
 
 @dramatiq.actor(queue_name="uws", priority=0)
 def job_started(job_id: str, message_id: str, start_time: str) -> None:
-    """Wrapper around the UWS function to mark a job as started.
+    """Call the UWS function to mark a job as started.
 
     Notes
     -----
@@ -113,26 +113,30 @@ def job_started(job_id: str, message_id: str, start_time: str) -> None:
     """
     logger = structlog.get_logger(config.logger_name)
     start = parse_isodatetime(start_time)
-    assert broker.worker_session, "Worker database connection not initalized"
-    assert start, f"Invalid start timestamp {start_time}"
+    if not broker.worker_session:
+        raise RuntimeError("Worker database connection not initalized")
+    if not start:
+        raise RuntimeError(f"Invalid start timestamp {start_time}")
     uws_job_started(job_id, message_id, start, broker.worker_session, logger)
 
 
 @dramatiq.actor(queue_name="uws", priority=10)
 def job_completed(
     message: dict[str, Any], result: list[dict[str, str]]
 ) -> None:
-    """Wrapper around the UWS function to mark a job as completed."""
+    """Call the UWS function to mark a job as completed."""
     logger = structlog.get_logger(config.logger_name)
     job_id = message["args"][0]
-    assert broker.worker_session, "Worker database connection not initalized"
+    if not broker.worker_session:
+        raise RuntimeError("Worker database connection not initalized")
     uws_job_completed(job_id, result, broker.worker_session, logger)
 
 
 @dramatiq.actor(queue_name="uws", priority=20)
 def job_failed(message: dict[str, Any], exception: dict[str, str]) -> None:
-    """Wrapper around the UWS function to mark a job as errored."""
+    """Call the UWS function to mark a job as errored."""
     logger = structlog.get_logger(config.logger_name)
     job_id = message["args"][0]
-    assert broker.worker_session, "Worker database connection not initalized"
+    if not broker.worker_session:
+        raise RuntimeError("Worker database connection not initalized")
     uws_job_failed(job_id, exception, broker.worker_session, logger)

src/vocutouts/broker.py

@@ -11,8 +11,6 @@
 
 from __future__ import annotations
 
-from typing import Optional
-
 import dramatiq
 import structlog
 from dramatiq import Broker, Middleware, Worker
@@ -33,7 +31,7 @@
 results = RedisBackend(host=config.redis_host, password=config.redis_password)
 """Result backend used by UWS."""
 
-worker_session: Optional[scoped_session] = None
+worker_session: scoped_session | None = None
 """Shared scoped session used by the UWS worker."""
 
 

src/vocutouts/cli.py

@@ -6,6 +6,7 @@
 import structlog
 import uvicorn
 from safir.asyncio import run_with_asyncio
+from safir.click import display_help
 from safir.database import create_database_engine, initialize_database
 
 from .config import config
@@ -15,28 +16,15 @@
 @click.group(context_settings={"help_option_names": ["-h", "--help"]})
 @click.version_option(message="%(version)s")
 def main() -> None:
-    """vo-cutouts main.
-
-    Administrative command-line interface for vo-cutouts.
-    """
-    pass
+    """Administrative command-line interface for vo-cutouts."""
 
 
 @main.command()
 @click.argument("topic", default=None, required=False, nargs=1)
 @click.pass_context
 def help(ctx: click.Context, topic: str | None) -> None:
     """Show help for any command."""
-    # The help command implementation is taken from
-    # https://www.burgundywall.com/post/having-click-help-subcommand
-    if topic:
-        if topic in main.commands:
-            click.echo(main.commands[topic].get_help(ctx))
-        else:
-            raise click.UsageError(f"Unknown help topic {topic}", ctx)
-    else:
-        assert ctx.parent
-        click.echo(ctx.parent.get_help())
+    display_help(main, ctx, topic)
 
 
 @main.command()
@@ -55,7 +43,7 @@ def run(port: int) -> None:
     "--reset", is_flag=True, help="Delete all existing database data."
 )
 @run_with_asyncio
-async def init(reset: bool) -> None:
+async def init(*, reset: bool) -> None:
     """Initialize the database storage."""
     logger = structlog.get_logger(config.logger_name)
     engine = create_database_engine(