refactor: add type hinting to data processors - #331
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the queens.data_processors package to add/strengthen type hints across multiple concrete DataProcessor implementations, and updates tooling configuration so these modules are included in static type checking.
Changes:
- Added/updated type annotations and docstrings for
DataProcessorand several file-based processors (CSV/NumPy/PVD/TXT). - Improved a small piece of TXT parsing robustness (avoid failure when the logger prefix regex doesn’t match a line).
- Updated mypy/pre-commit exclude patterns to stop excluding
src/queens/data_processorsfrom checks.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/queens/data_processors/txt_file.py | Adds type hints and makes logger-prefix stripping resilient to non-matching lines. |
| src/queens/data_processors/pvd_file.py | Adds type hints for PVD reading/filtering and Path-like parameters. |
| src/queens/data_processors/numpy_file.py | Adds type hints for NumPy loading and Path-like parameters. |
| src/queens/data_processors/csv_file.py | Adds type hints and refactors filtering variable names; introduces stricter filter config typing. |
| src/queens/data_processors/_data_processor.py | Adds type hints to the abstract base processor and helper methods. |
| src/queens/data_processors/init.py | Adds typing improvements to lazy-import __getattr__ and postpones annotation evaluation. |
| pyproject.toml | Adjusts mypy exclude list to include data_processors in type checking. |
| .pre-commit-config.yaml | Keeps pre-commit’s mypy exclude list consistent with pyproject (include data_processors). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Nice, type hinting makes coding so much nicer! I can look at it tomorrow probably. In the meantime, I think we have to be careful here with the merge order of #323: either you or me need to incorporate the changes the other PR made. |
Very good point! I think we should merge #323 first, so it doesn't become even bigger. @queens-py/developers, could anyone take another look at #323 and review it? |
leahaeusel
left a comment
There was a problem hiding this comment.
Thank you very much for reviewing this extensive PR! I will also add the missing override decorators shortly.
0b4f49b to
c5ec27b
Compare
ff53dca to
7d2f151
Compare
rjoussen
left a comment
There was a problem hiding this comment.
The remaining comments are not really about type hinting anymore, I guess. So I don't know if they should be changed here or not. But they definitely need to be addressed soon.
2fbb02f to
9983ed4
Compare
…le data processors
9983ed4 to
d10b148
Compare
Description and Context:
What and Why?
This PR adds type hinting to the data processors.
Related Issues and Pull Requests
Interested Parties