Skip to content

chore: pin lint tooling + normalize source and hand-fixed test files (1/2)#677

Merged
igerber merged 2 commits into
mainfrom
chore/lint-normalization
Jul 12, 2026
Merged

chore: pin lint tooling + normalize source and hand-fixed test files (1/2)#677
igerber merged 2 commits into
mainfrom
chore/lint-normalization

Conversation

@igerber

@igerber igerber commented Jul 12, 2026

Copy link
Copy Markdown
Owner

Summary

  • Part 1 of 2 of the repo-wide lint normalization, split so each PR fits the AI reviewer's input cap (the combined ~1.18MB diff exceeded the 1MB Codex input limit and crashed the review run; the mechanical-only remainder is PR 2/2). Together the two PRs reproduce the original change byte-for-byte.
  • Pin the lint toolchain exactly in the dev extra (black==26.3.1, ruff==0.15.13, mypy==2.1.0) so every machine and the upcoming Lint CI workflow judge cleanliness identically. The tools require Python >= 3.10 (dev tooling only; the library floor stays 3.9).
  • Scoped [tool.ruff.lint.per-file-ignores] for deliberate patterns, each with a why-comment: trop* logger-before-imports (E402), honest_did Rambachan-Roth math notation l (E741), __init__ backend re-export surface (F401), conftest sys.path ordering (E402), tutorial-quoted CdH horizon notation (E741).
  • Mechanical normalization of diff_diff/ source (commit 1): verbatim ruff check diff_diff --fix + black diff_diff output at the pinned versions.
  • The 16 test files with non-autofixable findings (commit 2): their mechanical reformat plus hand fixes - 2 latent F821s (typing names used in an annotation without import), 1 import hoist (E402), 1 rename (E741), and 26 individually audited F841 unused locals, two of which were real never-asserted test bugs now upgraded to behavioral assertions (test_trends_nonparam_basic asserts r_plain finiteness; test_influence_function_normalization drops an abandoned R-comparison block and gets an honest docstring).

Reviewer note: commit 1's diff_diff/ changes are byte-reproducible with the two commands above; all judgment calls are in commit 2.

Methodology references (required if estimator / math changes)

  • Method name(s): N/A - no methodology changes (formatting/lint only; mypy error count verified byte-identical at 184 pre/post, confirming no semantic drift in diff_diff/)
  • Paper / source link(s): N/A
  • Any intentional deviations from the source (and why): None

Validation

  • Tests added/updated: no new tests; 16 test files hand-fixed. Targeted runs green across all touched areas, including the full tests/test_openai_review.py -m '' (250 passed).
  • Backtest / simulation / notebook evidence (if applicable): N/A

Security / privacy

  • Confirm no secrets/PII in this PR: Yes

Generated with Claude Code

🤖 Generated with Claude Code

https://claude.ai/code/session_01Lbd6nqWmg4snvvBmegwqiw

@igerber

igerber commented Jul 12, 2026

Copy link
Copy Markdown
Owner Author

/ai-review

igerber and others added 2 commits July 12, 2026 11:08
pyproject: exact pins (black==26.3.1, ruff==0.15.13, mypy==2.1.0) in the
dev extra + scoped per-file-ignores for deliberate patterns. diff_diff/
changes are the verbatim output of `ruff check diff_diff --fix` +
`black diff_diff` at the pinned versions. mypy error count verified
unchanged (184) pre/post.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lbd6nqWmg4snvvBmegwqiw
…ndings

Each of these files carries its mechanical black/ruff-fix changes PLUS the
hand fixes:
- F821: test_methodology_sdid.py missing typing imports for a local
  annotation (latent NameError trap, not runtime)
- E402: test_rust_backend.py mid-file _rust_demean_map import hoisted
- E741: test_doc_snippets.py comprehension variable l -> ln
- F841 x26 audited individually:
  * test_chaisemartin_dhaultfoeuille trends_nonparam_basic: r_plain was
    computed but never asserted despite the comment promising both results
    finite -> added the missing finiteness assertions
  * test_se_accuracy influence_function_normalization: deleted the
    abandoned R-style comparison block and rewrote the docstring to state
    what the test actually asserts
  * test_imputation always_treated_warning: removed the dead first two
    data-construction attempts (kept the explanatory note)
  * remaining sites: dropped dead locals, keeping side-effectful calls

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lbd6nqWmg4snvvBmegwqiw
@igerber igerber changed the title chore: repo-wide lint normalization + pinned lint tooling chore: pin lint tooling + normalize source and hand-fixed test files (1/2) Jul 12, 2026
@igerber igerber force-pushed the chore/lint-normalization branch from f4f1824 to 6d77942 Compare July 12, 2026 15:09
@github-actions

Copy link
Copy Markdown

🔁 AI review rerun (requested by @igerber)

Head SHA: 6d77942b907965d21142847572a05b6494a3ee8c


Overall Assessment

Looks good — no unmitigated P0 or P1 findings.

Executive Summary

  • The changed library source appears limited to import ordering, formatting, and unused-import cleanup.
  • I found no estimator math, weighting, variance/SE, identification assumption, or default behavior changes.
  • Methodology registry/TODO conventions were checked; no new undocumented methodology deviation is introduced.
  • diff_diff source files touched by the PR parse cleanly with ast.parse.
  • Runtime import/tests could not be run in this sandbox because numpy is not installed.

Methodology

No findings.

Affected methods: none. The estimator-touching files in the supplied diff do not alter formulas, SE paths, inference gates, weights, assumptions, or defaults. The HonestDiD reference remains unchanged in diff_diff/honest_did.py:L1-L17, and the registry remains the controlling methodology source at docs/methodology/REGISTRY.md:L1-L5.

Code Quality

No findings.

The suspected import/syntax issue in diff_diff/__init__.py is not present; the initializer is structurally valid around the reordered exports at diff_diff/__init__.py:L23-L303. The removed local warnings import in TROP is safe because the module-level import remains at diff_diff/trop_local.py:L13-L15.

Performance

No findings.

The source changes do not introduce new loops, allocations, estimator execution paths, or backend dispatch changes beyond mechanically split imports.

Maintainability

  • Severity: P3 informational
    Impact: The dev extra now pins tools that are documented as requiring Python >= 3.10 while the package runtime still targets Python 3.9. This is already called out in both config and changelog, so it is not a blocker.
    Concrete fix: No required change. Optional: split lint/type tooling into a lint extra or contributor-doc note if Python 3.9 contributors need .[dev] to install cleanly.
    Location: pyproject.toml:L63-L69, CHANGELOG.md:L11-L17

Tech Debt

No findings.

No new deferred methodology, correctness, or test debt is introduced by the reviewed diff. Existing deferred items remain tracked under TODO.md:L50-L80.

Security

No findings.

The supplied diff does not add secrets, credential handling, network behavior, or code execution surfaces.

Documentation/Tests

  • Severity: P3 informational
    Impact: I could not independently run the test suite or import smoke test in this sandbox because numpy is unavailable. I did verify the touched source files parse cleanly in-memory.
    Concrete fix: Ensure CI runs the touched test set in a dependency-complete environment. No PR code change required.

@igerber igerber added the ready-for-ci Triggers CI test workflows label Jul 12, 2026
@igerber igerber merged commit faf10b3 into main Jul 12, 2026
41 of 42 checks passed
@igerber igerber deleted the chore/lint-normalization branch July 12, 2026 16:48
igerber added a commit that referenced this pull request Jul 12, 2026
…es (#679)

Verbatim output of `ruff check tests --fix` + `black tests` at the pinned
versions (black==26.3.1, ruff==0.15.13; pinned in #677) for the 37 test
files with no hand edits: import sorts, f-string prefixes, unused-import
removal, formatting. No assertion or fixture changes.


Claude-Session: https://claude.ai/code/session_01Lbd6nqWmg4snvvBmegwqiw

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
igerber added a commit that referenced this pull request Jul 12, 2026
…docs

- .github/workflows/lint.yml: runs `ruff check` + `black --check` at the
  pyproject-pinned versions on every PR push and on pushes to main. No
  ready-for-ci gate (precedent: ai_pr_review.yml), no paths filter (a
  required check whose workflow never triggers would block PRs forever).
  Aggregate "Lint Gate" job (if: always(), inspects needs.*.result) gives
  branch protection a single stable required-check name. Check-only.
- .git-blame-ignore-revs: the #677/#679 normalization squash SHAs.
- CONTRIBUTING.md "Linting and Formatting" section + CLAUDE.md note.
- TestLintWorkflowPinSync: executable ruff/black pin-sync guard between
  lint.yml and the pyproject dev extra (review round-2 P3).
- lint.yml added to rust-test.yml push/pull_request path filters, locked
  by PIN_SYNC_WORKFLOWS assertions; docs distinguish the current
  ruff/black mirroring from the future mypy CI job (round-3 P3s).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lbd6nqWmg4snvvBmegwqiw
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-ci Triggers CI test workflows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant