Skip to content

test(lint): flag PEP-604 unions without __future__ guard in src/*.py (closes #961 Option A) - #1188

Closed
bassilkhilo-ag2 wants to merge 1 commit into
sonichi:mainfrom
bassilkhilo-ag2:test/py39-compat-lint-961
Closed

bassilkhilo-ag2 wants to merge 1 commit into
sonichi:mainfrom
bassilkhilo-ag2:test/py39-compat-lint-961

Conversation

@bassilkhilo-ag2

Copy link
Copy Markdown
Collaborator

Summary

Adds tests/py39-compat.test.py — an AST-based lint that catches the #960-class bug (telegram-bridge outage from missing __future__ import) before it merges.

What it checks: Every src/*.py file that uses PEP-604 X | Y union syntax in annotation positions (function args, return types, variable annotations) must have from __future__ import annotations at module level. If not, the file crashes on import under Python 3.9 (macOS stock CLT interpreter).

Scope: Only src/*.py — skills and tests are typically invoked under Homebrew Python where 3.10+ is available.

Why Option A (lint) instead of Option B (baseline bump)

Option B (changing startup.sh to resolve a 3.10+ interpreter) is the cleaner long-term answer per issue #961, but it's a launch-path change across startup.sh and all bare-python3 call sites — a deliberate migration. Option A is a 1-file addition that catches regressions regardless of which option eventually wins.

This lint unblocks the decision: if owner picks A, this is the implementation. If owner picks B, this lint is a belt-and-suspenders safety net during the transition.

Current state

27 src/*.py files checked — all pass (all current files that use | unions already have the guard).

Test plan

python3 tests/py39-compat.test.py  # → PASS: 27 src/*.py files checked

Verify regression detection works:

# Temporarily add to a src/*.py without the __future__ import:
def foo(x: str | None) -> None: pass
# → FAIL: flagged by lint

🤖 Generated with Claude Code

…/*.py (closes sonichi#961 Option A)

Adds a lint test that AST-walks src/*.py, identifies any file that uses
X | Y type-union syntax in annotation positions (function args, return
annotations, variable annotations), and fails if `from __future__ import
annotations` is absent.

Python 3.9 (the macOS CLT interpreter) can't evaluate PEP-604 unions at
runtime. The telegram-bridge outage in sonichi#960 was caused by a missing
__future__ import. This lint catches the same bug class pre-merge.

This is Option A of the issue sonichi#961 decision (lint guard vs baseline bump).
Option B (startup.sh 3.10+ resolver) can stack on top if owner prefers;
this guard prevents regressions in either case.

27 src/*.py files checked, all currently passing.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

@VasiliyRad VasiliyRad left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint rule is correct: AST walk covers all annotation positions (function args, posonlyargs, kwonlyargs, vararg, kwarg, returns, AnnAssign). Logic is sound — only flags when future guard is absent AND PEP-604 BinOp|BitOr appears in annotation nodes. Single-file addition, no unrelated changes. Passes clean against all 27 current src/*.py files.

@VasiliyRad VasiliyRad left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionally correct, but this overlaps almost entirely with #1199's scripts/check-py39-union-annotations.py + CI wiring — same goal (Option A of #961), same logic (AST walk for BitOr in annotation positions, gated on from __future__ import annotations), same allowed/disallowed set.

If #1199 lands, this test file is redundant — the script is the gate, and CI invokes it. The only marginal value of keeping both would be npm run test:py (or similar local runner) picking it up. If that's the intent, fine — but I'd rather see one canonical implementation. Suggest closing this PR and letting #1199 carry the gate, or vice versa.

Not blocking on the redundancy alone if you want both for belt-and-suspenders, but please confirm the intent before landing.

@bassilkhilo-ag2

Copy link
Copy Markdown
Collaborator Author

Closing — PR #1199 is the more complete solution for #961: dedicated scripts/check-py39-union-annotations.py + explicit CI step + companion deal-finder fix. VasiliyRad flagged the overlap in the review here. #1199 covers the same AST-walk logic and has two approvals.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants