[CI]: Enforce lazy export synchronization - #174
[CI]: Enforce lazy export synchronization#174Spencer Schoenberg (spencrr) wants to merge 1 commit into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Nina Chikanov (nina-msft)
left a comment
Nina Chikanov (nina-msft)
left a comment
There was a problem hiding this comment.
Both coverage nits flagged by Copilot - lmk what you think!
| for statement in tree.body: | ||
| if ( | ||
| isinstance(statement, ast.Assign) | ||
| and any( | ||
| isinstance(target, ast.Name) and target.id == name | ||
| for target in statement.targets | ||
| ) | ||
| ) or ( | ||
| isinstance(statement, ast.AnnAssign) | ||
| and isinstance(statement.target, ast.Name) | ||
| and statement.target.id == name | ||
| ): | ||
| value = statement.value | ||
| return value |
There was a problem hiding this comment.
This comment was generated by Copilot.
Likelihood: low. Today __all__ is only ever built as a single literal assignment, so this can't misfire right now. It becomes a real risk only if someone later constructs __all__ (or __lazy_imports__) incrementally — which is a fairly common pattern, so worth guarding before it can silently bite.
Finding: _module_assignment only tracks Assign/AnnAssign and keeps the last one, so it silently mis-analyzes augmented construction of these names:
__all__ = ["Other"]
__all__ += ["Heavy"] # or __all__.extend([...]) / .append(...)Here it captures only {"Other"} and then flags Heavy as missing — a false positive. The concern is the asymmetry: fully dynamic forms are rejected loudly (RMP002_DYNAMIC_ALL / RMP002_DYNAMIC_REGISTRY), but += / .extend() / .append() on __all__ or __lazy_imports__ is miscounted silently. Since the checker runs repo-wide, I'd rather it fail loud than wrong.
Consider: detect an AugAssign — and an attribute-call mutation (.extend/.append/.update) — targeting either name and treat it as dynamic, yielding the existing "must be a literal" violation.
| assert message.startswith("RMP002") | ||
| assert "`Heavy`" in message | ||
|
|
||
| def test_accepts_lazy_export_in_all(self) -> None: |
There was a problem hiding this comment.
This comment was generated by Copilot.
Likelihood: low — these are coverage nice-to-haves, not a sign of a current bug; the existing tests already exercise the main paths. Worth adding mainly to pin down intent against future regressions.
Two cases worth adding:
- Mixed registry — several lazy keys where only some are in
__all__; assert exactly the missing name(s) are reported (guards against an all-or-nothing regression). - The
__all__ += [...]/.extend(...)augmented form, asserting whatever behavior you settle on above — so the intent is documented either way.
2 participants
Description
Follow-up to #173.
Add a repository-specific Flake8 rule that keeps PEP 562 lazy export registries synchronized with the public module API.
RMP002to require every literal__lazy_imports__key in__all__.__all__expressions.AsyncSuffixChecker(RMP001) andLazyExportChecker(RMP002).--selectcoverage for both rules.RMP002as an external Ruff code.Validation:
python -m pre_commit run --all-filesBreaking changes
None.
Checklist
pre-commit run --all-filespasses