fix: typing issues#1
Conversation
introduces a new _bindings.pyi as well as typing.py to allow for all types to check and run
|
Looks like a good idea, thanks! I will have copilot to an automated review first and then look through the changes myself later. |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces comprehensive type annotations and a new stub file (_bindings.pyi) to enable strict type checking with pyright. The changes include adding type hints throughout the codebase, creating a typing module for common geometry types, reformatting code with ruff, and configuring pyright in pyproject.toml.
- Added type annotations to all functions, methods, and class attributes across the codebase
- Created
_bindings.pyistub file for CGAL geometry bindings andtyping.pyfor common type aliases - Fixed a bug where
iswas incorrectly used instead ofinfor dictionary membership checking - Reformatted code for improved consistency (spacing, line breaks, unused variable handling)
Reviewed Changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/cgshop2026_pyutils/geometry/_bindings.pyi | New stub file providing type signatures for C++ CGAL bindings |
| src/cgshop2026_pyutils/geometry/typing.py | New module defining type aliases for Edge, Triangle, and flip sequences |
| src/cgshop2026_pyutils/geometry/init.py | Added exports for new typing aliases and FieldNumber |
| src/cgshop2026_pyutils/geometry/flippable_triangulation.py | Added type annotations, @override decorators, and proper handling of unused return values |
| src/cgshop2026_pyutils/geometry/flip_partner_map.py | Added type annotations for class attributes and method parameters |
| src/cgshop2026_pyutils/geometry/draw.py | Added type hint for Axes parameter |
| src/cgshop2026_pyutils/zip/zip_writer.py | Added type annotations including TracebackType for __exit__ method |
| src/cgshop2026_pyutils/zip/zip_reader_errors.py | Added type annotations and used TYPE_CHECKING to avoid circular imports |
| src/cgshop2026_pyutils/zip/zip_processor.py | Added comprehensive type annotations and @override decorator |
| src/cgshop2026_pyutils/io/init.py | Added generic types for file-like objects and type-safe decorator |
| src/cgshop2026_pyutils/instance_database/*.py | Added type annotations, @override decorators, and fixed is vs in bug |
| src/cgshop2026_pyutils/schemas/solution.py | Changed meta type from untyped dict to dict[str, Any] |
| src/cgshop2026_pyutils/visualize.py | Added Axes type hint and pyright ignore comment |
| src/cgshop2026_pyutils/verify.py | Reformatted for better readability |
| tests/*.py | Added explicit type annotations for empty lists to help type checker, replaced unused variables with underscores |
| pyproject.toml | Added pyright configuration (with syntax error) |
| example*.ipynb | Reformatted for consistent spacing and line breaks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| file_size_limit=file_size_limit, zip_size_limit=zip_size_limit | ||
| ) | ||
| self._solution_extensions = solution_extensions | ||
| self._solution_extensions: tuple[str, str] = solution_extensions |
There was a problem hiding this comment.
The self._solution_extensions attribute is typed as tuple[str, str], which is a fixed-size tuple of exactly 2 strings. This should match the parameter type and allow any number of extensions using tuple[str, ...] or Sequence[str].
| @override | ||
| def __ne__(self, other: object) -> bool: ... |
There was a problem hiding this comment.
__ne__ is automatically defined when __eq__ is defined in Python 3. The explicit @override decorator here is incorrect since there's no need to override __ne__ - it's synthesized from __eq__. Consider removing this method definition.
| @override | |
| def __ne__(self, other: object) -> bool: ... |
There was a problem hiding this comment.
well, this is from L55 of _bindings.cpp
.def(py::self != Point())| file_size_limit: int = 250 * 1_000_000, # 250 MB file size limit | ||
| zip_size_limit: int = 2_000 * 1_000_000, # 2 GB zip size limit | ||
| solution_extensions=(".solution.json", ".sol.json"), | ||
| solution_extensions: tuple[str, str] = (".solution.json", ".sol.json"), |
There was a problem hiding this comment.
The solution_extensions parameter is typed as tuple[str, str], which is a fixed-size tuple of exactly 2 strings. This is overly restrictive - it should be tuple[str, ...] to allow any number of extensions, or Sequence[str] for better flexibility.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
(apologies for the massive diff, this is a result of squashing a few commits on a private fork) |
Don't worry. The review tool in GitHub works pretty well with such things and only show me the actual changes, not the individual commits. :) |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
I didn't have the test CI available for PRs. Seems like you have been using Python >=3.13 features which broke the package for older Python versions. See #2 for how I am fixing it. |
Introduces a new _bindings.pyi as well as typing.py to allow for all types to check and run using a strict typechecker (pyright). It is slightly opionated to allow for most code to remain as is, while removing most of the common but unneeded type errors (like unused call result, or implicit string concatenation).
I have also run the entire codebase through ruff. I would be happy to incorporate any other changes that you might find necessary