Merge pull request #656 from thomwebb/fix/i18n-audit-false-positives

fix(i18n/audit): eliminate false positives in raw-site classification

Author: mpfaffenberger

code_puppy/i18n/audit.py

@@ -136,11 +136,38 @@ def _is_translation_call(node: ast.expr) -> bool:
 
 
 def _has_string_literal(node: ast.expr) -> bool:
-    """True if the expression *contains* a hard-coded string literal."""
+    """True if the expression *contains* a hard-coded string literal.
+
+    For f-strings we only return True when at least one constant segment
+    contains non-whitespace text.  Pure-variable f-strings like ``f"{x}"``
+    or ``f"  {x}"`` carry no translatable literal and are classified as
+    dynamic instead.
+    """
     if isinstance(node, ast.Constant):
         return isinstance(node.value, str)
     if isinstance(node, ast.JoinedStr):  # f-string
-        return True
+        # An f-string is only "raw" when it has at least one constant part
+        # with meaningful (non-whitespace) text, e.g. f"Error: {e}".
+        # Pure-variable forms like f"{var}" or f"  {var}" are dynamic.
+        #
+        # Also recurse into ``FormattedValue.value`` so a wrapped literal
+        # like ``f"{'Error: connection refused'}"`` (which parses as
+        # ``JoinedStr([FormattedValue(Constant('...'))])``) is still
+        # classified as raw instead of being dropped as dynamic.
+        return any(
+            (
+                isinstance(v, ast.Constant)
+                and isinstance(v.value, str)
+                and v.value.strip()
+            )
+            or (
+                isinstance(v, ast.FormattedValue)
+                and isinstance(v.value, ast.Constant)
+                and isinstance(v.value.value, str)
+                and v.value.value.strip()
+            )
+            for v in node.values
+        )
     if isinstance(node, ast.BinOp):  # "a" + x, "a" % x
         return _has_string_literal(node.left) or _has_string_literal(node.right)
     if isinstance(node, ast.Call):
@@ -198,6 +225,16 @@ def audit_source(source: str, path: str) -> List[Site]:
 
 
 def _iter_py_files(root: str) -> Iterable[str]:
+    """Yield every .py file under ``root``.
+
+    If ``root`` is itself a ``.py`` file it is yielded directly so that
+    ``python -m code_puppy.i18n.audit path/to/module.py`` works as
+    expected instead of silently producing an empty report.
+    """
+    if os.path.isfile(root):
+        if root.endswith(".py"):
+            yield root
+        return
     for dirpath, dirnames, filenames in os.walk(root):
         dirnames[:] = [d for d in dirnames if d not in _SKIP_DIRS]
         for name in filenames:
@@ -206,7 +243,13 @@ def _iter_py_files(root: str) -> Iterable[str]:
 
 
 def audit_tree(root: str) -> Report:
-    """Audit every Python module under ``root``."""
+    """Audit every Python module under ``root``, or ``root`` itself when it is a ``.py`` file."""
+    if not os.path.isdir(root) and not os.path.isfile(root):
+        # Silent-zero is worse than useless — an empty Report has
+        # coverage == 100.0, so a typo'd path would sail past
+        # ``--fail-under`` and tell CI everything is fine. Fail loud:
+        # this is a config/programming error, not a data condition.
+        raise FileNotFoundError(f"audit root does not exist: {root!r}")
     report = Report()
     for path in sorted(_iter_py_files(root)):
         try:

tests/i18n/test_i18n_audit.py

@@ -1,5 +1,7 @@
 """Tests for the static i18n extraction audit (code_puppy.i18n.audit)."""
 
+import ast
+
 from code_puppy.i18n import audit
 
 
@@ -16,6 +18,47 @@ def test_fstring_is_raw():
     assert _kinds('emit_warning(f"hi {name}")') == ["raw"]
 
 
+def test_fstring_pure_variable_is_dynamic():
+    """f"{var}" has no literal content — must not be reported as raw."""
+    assert _kinds('emit_info(f"{result}")') == ["dynamic"]
+
+
+def test_fstring_whitespace_only_literal_is_dynamic():
+    """f"  {var}" has only whitespace in the constant part — not translatable."""
+    assert _kinds('emit_info(f"   {msg}")') == ["dynamic"]
+
+
+def test_fstring_with_content_and_variable_is_raw():
+    """f"Error: {e}" has a meaningful literal prefix — must stay raw."""
+    assert _kinds('emit_error(f"Error: {e}")') == ["raw"]
+
+
+def test_fstring_wrapped_literal_is_raw():
+    """f"{'literal'}" wraps a Constant inside a FormattedValue.
+
+    The classifier used to only inspect direct Constant children of
+    JoinedStr, so this parsed as ``JoinedStr([FormattedValue(Constant)])``
+    and was silently classified as dynamic — a false negative. Recurse
+    into FormattedValue.value so it now shows up as raw.
+    """
+    src = "emit_info(f\"{'Error: connection refused'}\")"
+    # Direct AST check so we're not relying on the higher-level pipeline.
+    tree = ast.parse(src, mode="eval")
+    call = tree.body
+    assert isinstance(call, ast.Call)
+    joined = call.args[0]
+    assert isinstance(joined, ast.JoinedStr)
+    assert isinstance(joined.values[0], ast.FormattedValue)
+    # And end-to-end through the classifier.
+    assert audit._classify(joined) == "raw"
+    assert _kinds(src) == ["raw"]
+
+
+def test_fstring_with_only_arrow_is_raw():
+    """Non-whitespace punctuation like an arrow counts as a literal."""
+    assert _kinds('emit_info(f"-> {item}")') == ["raw"]
+
+
 def test_string_concat_is_raw():
     assert _kinds('emit_error("bad: " + detail)') == ["raw"]
 
@@ -108,6 +151,57 @@ def test_json_output_is_valid(tmp_path, capsys):
     assert payload["coverage"] == 50.0
 
 
+def test_single_file_path_is_accepted(tmp_path, capsys):
+    """Passing a .py file directly must produce a non-empty report.
+
+    Previously ``_iter_py_files`` called ``os.walk(file)`` which yields
+    nothing, so every per-file audit silently returned 0 sites.
+    """
+    import json
+
+    mod = tmp_path / "solo.py"
+    mod.write_text('emit_info("raw string")\n', encoding="utf-8")
+    assert audit.main([str(mod), "--json"]) == 0
+    payload = json.loads(capsys.readouterr().out)
+    assert payload["raw"] == 1, "single-file audit must find the raw site"
+
+
+def test_single_file_pure_variable_fstring_not_raw(tmp_path, capsys):
+    """Single-file audit: f"{var}" must NOT be counted as raw."""
+    import json
+
+    mod = tmp_path / "mod.py"
+    mod.write_text('emit_info(f"{result}")\n', encoding="utf-8")
+    assert audit.main([str(mod), "--json"]) == 0
+    payload = json.loads(capsys.readouterr().out)
+    assert payload["raw"] == 0
+    assert payload["dynamic"] == 1
+
+
+def test_nonexistent_path_raises(tmp_path, capsys):
+    """A typo'd path must not silently report 100% coverage.
+
+    Previously ``audit_tree('/does/not/exist')`` returned an empty
+    ``Report`` whose ``coverage`` property is ``100.0``, so
+    ``--fail-under`` would happily pass on a typo'd path and tell CI
+    everything was fine. Fail loud instead: this is a
+    programming/config error, so ``FileNotFoundError`` propagates all
+    the way out of ``main()`` and produces a nonzero exit.
+    """
+    import pytest
+
+    missing = tmp_path / "totally-not-real"
+    assert not missing.exists()
+
+    # audit_tree raises directly.
+    with pytest.raises(FileNotFoundError):
+        audit.audit_tree(str(missing))
+
+    # And main() lets it propagate — no accidental swallow, no exit 0.
+    with pytest.raises(FileNotFoundError):
+        audit.main([str(missing)])
+
+
 # --- integration smoke ----------------------------------------------------
 def test_audits_the_real_package_without_error():
     """The tool must stay runnable against the live tree as it evolves.