-
Notifications
You must be signed in to change notification settings - Fork 67
chore(tidy3d): FXC-4559-add-type-checking-import-moving-and-checking-to-pre-commit-and-ci #3093
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: yaugenst-flex/pydantic-v2
Are you sure you want to change the base?
Conversation
chore(tidy3d): FXC-4545-move-type-hints-imports-behind-type-checking
96baf60 to
0157641
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5 files reviewed, 2 comments
…to-pre-commit-and-ci
0157641 to
d80cef2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5 files reviewed, no comments
| hooks: | ||
| - id: move-type-imports | ||
| name: move type-only imports under TYPE_CHECKING | ||
| entry: poetry run python scripts/move_type_imports.py --mode fix --only-changed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way we can do this without using poetry? Just because not everyone has it installed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it, should we just avoid it in the precommit but keep it in CI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was not expecting that...
Either we might skip it or do this one?
- repo: local
hooks:
- id: move-type-imports
name: move type-only imports under TYPE_CHECKING
entry: python scripts/move_type_imports.py --mode fix --only-changed
language: python
additional_dependencies:
- "libcst==1.4.0"
pass_filenames: false
stages: [ pre-commit ]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think this will work for people who don't even have python installed in the system? Just checking how standalone it is? Is it fully self contained?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It only requires python with the libcst package.
So it's not perfectly standalone, but I would not assume that these users use the pre-commit hook? I mean there is no force to do it...
| The script scans module-level imports and moves those referenced only in | ||
| annotations or existing ``if TYPE_CHECKING`` blocks into a consolidated | ||
| ``if TYPE_CHECKING:`` section. Imports used in class-level annotations are | ||
| left in place to avoid breaking pydantic field evaluation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this is part of a wider conversation about how we do dependency managements and typing. For now sounds good to use it but in the long run we can evaluate how we do this between the planned packages
5293ae1 to
6f85dd4
Compare
1332caa to
32e90e7
Compare
6cc521f to
95adf41
Compare
looks good (see move-type-imports):
https://github.com/flexcompute/tidy3d/actions/runs/20262172636/job/58176641591
Greptile Overview
Greptile Summary
Added automated tooling to enforce
TYPE_CHECKINGguards for type-only imports across the codebase. The implementation includes a new script that analyzes Python files to identify imports used solely in type annotations and automatically moves them behindif TYPE_CHECKING:blocks. This optimization reduces runtime import overhead while maintaining type checking correctness.Key changes:
scripts/move_type_imports.pywith fix mode (auto-repairs) and check mode (validation only)Anyfrom typing, and allows skip commentstidy3d/web/tests/conftest.pyas demonstration, movingGeneratorimport behindTYPE_CHECKINGConfidence Score: 5/5
Important Files Changed
File Analysis
TYPE_CHECKINGguards with both fix and check modesmove-type-importsto verify type-only imports are properly guarded, integrated into workflow status checksGeneratorimport behindTYPE_CHECKINGguard as it's only used in type annotationsSequence Diagram
sequenceDiagram participant Dev as Developer participant PreCommit as Pre-commit Hook participant Script as move_type_imports.py participant CI as GitHub Actions participant Check as Check Mode Note over Dev,Check: Local Development Flow Dev->>Dev: Modify Python files Dev->>PreCommit: git commit PreCommit->>Script: --mode fix --only-changed Script->>Script: Analyze changed files Script->>Script: Move type-only imports to TYPE_CHECKING Script->>Dev: Auto-fix files Dev->>Dev: Commit succeeds with fixed imports Note over Dev,Check: CI Validation Flow Dev->>CI: Push to PR CI->>CI: determine-test-scope CI->>Check: Run move-type-imports job Check->>Script: --mode check_on_change Script->>Script: Analyze all tidy3d files Script->>Script: Check for unguarded type imports alt All imports properly guarded Script->>Check: Exit 0 (success) Check->>CI: Job passes else Type imports not guarded Script->>Check: Exit 1 with error list Check->>CI: Job fails CI->>Dev: PR check fails end