Vamsi-klu commented on code in PR #73076: URL: https://github.com/apache/airflow/pull/73076#discussion_r4002488548
########## scripts/ci/prek/check_exception_format_args.py: ########## @@ -0,0 +1,235 @@ +#!/usr/bin/env python3 +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# /// script +# requires-python = ">=3.10" +# dependencies = [ +# "rich>=13.0.0", +# ] +# /// +"""Check that no new ``raise SomeError("... %s ...", value)`` usages are introduced. + +Exception constructors do not interpolate their arguments the way ``logger`` +calls do -- ``Exception.__init__`` just stores everything in ``args``. So:: + + raise AirflowException("TaskInstance %s is not found", ti.task_id) + +renders as ``('TaskInstance %s is not found', 'my_task')`` rather than the +intended sentence, and ``AirflowException.serialize()`` carries that same +``str(self)`` across the trigger/task boundary. Exceptions whose ``__init__`` +forwards only the message to ``super()`` -- ``google.api_core``'s +``GoogleAPICallError`` family, for instance -- drop the trailing arguments +outright, so the identifier the message exists to carry never reaches the user. +Bind an f-string to a variable and raise that instead. + +Detection is AST-based because the pattern routinely spans several lines, and a +call is only flagged when the number of ``%`` placeholders in the leading string +literal exactly matches the number of trailing arguments -- counted the way +Python's own ``%`` operator would consume them, flags and width and precision +included. Anything looser misfires on the ten or so places that already pass a +literal message alongside unrelated positional parameters, such as +``TypeError("Could not parse hits.", response)`` in the elasticsearch provider or +SQLAlchemy's ``OperationalError(statement, params, orig)``. + +Precision costs some recall, deliberately. The check sees only an exception +raised inline, so ``err = ValueError("%s", x); raise err`` slips past, as does +any call whose keyword arguments make the count disagree. Dict-style +``%(name)s`` and ``*`` widths are skipped outright: both consume arguments in a +way a plain count cannot describe. + +All *existing* usages are recorded in ``generated/known_exception_format_args.txt`` +as ``relative/path::N`` entries (one per file), where ``N`` is the maximum number +of occurrences allowed in that file. A file whose current count exceeds the +recorded limit is treated as a violation. + +Modes +----- +Default (files passed by prek/pre-commit): + Check only the supplied files; fail if any file's count exceeds the limit. + When a file's count has *decreased*, the allowlist entry is tightened + automatically and the hook exits with a non-zero code so that pre-commit + reports the modified allowlist -- just stage + ``generated/known_exception_format_args.txt`` and re-run. + +``--all-files``: + Walk the whole repository and check every ``.py`` file. + +``--cleanup``: + Remove entries for files that no longer exist. Safe to run at any time; + does not add new entries or raise limits. + +``--generate``: + Scan the whole repository and *rebuild* the allowlist from scratch. + Intended for the initial setup or after a large-scale clean-up sprint. +""" + +from __future__ import annotations + +import argparse +import ast +import re +from collections.abc import Iterable +from pathlib import Path + +from common_prek_utils import AIRFLOW_ROOT_PATH, AllowlistManager +from rich.console import Console + +console = Console(color_system="standard", width=200) + +REPO_ROOT = AIRFLOW_ROOT_PATH + +_FORMAT_TOKEN_RE = re.compile( + r""" + % + (?: + % # an escaped literal percent + | (?P<mapping>\([^)]*\))? # mapping key, for dict-style formatting + [#0\- +]* # flags + (?:\*|\d+)? # minimum field width + (?:\.(?:\*|\d+))? # precision + [hlLqjzt]? # length modifier, accepted and ignored + (?P<conversion>[diouxXeEfFgGcrsa]) Review Comment: The regex uses Python % rules, including a space flag. Your own test already encodes this: count_positional_placeholders("50% off") == 1. So raise ValueError("download is 50% complete", response) or raise ValueError("50% off", extra) fails the hook even though nobody meant formatting. None of the current allowlisted sites are this shape. The next person who writes a percent in English and passes a context object gets a red CI and a --generate hint that widens the whole list. Document that, or skip a % whose conversion is glued to a following word. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
