feat: enable mypy session for documentai-toolbox#16690
feat: enable mypy session for documentai-toolbox#16690chalmerlowe wants to merge 17 commits intomainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the mypy type checking session in the noxfile.py, replacing a previous skip command. Feedback indicates that enabling this session immediately may cause CI failures due to existing type errors and recommends using a configuration file or adding error codes for better visibility. Additionally, the specified mypy version constraint was noted as referencing an unreleased version.
|
/gemini review |
| if gcs_uri: | ||
| gcs_bucket_name, gcs_prefix = split_gcs_uri(gcs_uri) | ||
|
|
||
| assert gcs_prefix is not None |
There was a problem hiding this comment.
We should avoid using assert outside of tests
There was a problem hiding this comment.
Code Review
This pull request enables mypy type checking in the noxfile and addresses various type-related issues across the package. Key changes include adding null checks for bounding box coordinates, updating type hints to use more flexible types like Iterable and TypeVar, and ensuring proper instantiation of vision.Vertex objects. While these changes improve overall type consistency, the feedback highlights that several modifications in block.py replace specific types with Any, which significantly reduces type safety. Additionally, a redundant type cast was identified in the load_blocks_from_schema function.
|
|
||
|
|
||
| def _get_target_object(json_data: any, target_object: str) -> Optional[SimpleNamespace]: | ||
| def _get_target_object(json_data: Any, target_object: str) -> Any: |
There was a problem hiding this comment.
The return type of _get_target_object has been changed to Any, which significantly reduces type safety. Given that the docstring explicitly states it returns a SimpleNamespace, it would be better to use Optional[SimpleNamespace] or a more specific type if possible. If this is a temporary measure to resolve mypy errors, please consider adding a TODO to refine this type later.
There was a problem hiding this comment.
Locally Gemini and I have gone round and round today trying to find the ideal collection of types so that mypy passes and so that unittests pass.
What we have now is not necessarily ideal, but it is likely better than what we had and further churn may not be cost effective.
| type_: Any = dataclasses.field(init=True, repr=False) | ||
| text: Any = dataclasses.field(init=True, repr=False) | ||
| bounding_box: Any = dataclasses.field(init=True, repr=False, default=None) | ||
| block_references: Any = dataclasses.field(init=True, repr=False, default=None) | ||
| block_id: Any = dataclasses.field(init=False, repr=False, default=None) | ||
| confidence: Any = dataclasses.field(init=False, repr=False, default=None) | ||
| page_number: Any = dataclasses.field(init=False, repr=False, default=None) | ||
| page_width: Any = dataclasses.field(init=False, repr=False, default=None) | ||
| page_height: Any = dataclasses.field(init=False, repr=False, default=None) | ||
| bounding_width: Any = dataclasses.field(init=False, repr=False, default=None) | ||
| bounding_height: Any = dataclasses.field(init=False, repr=False, default=None) | ||
| bounding_type: Any = dataclasses.field(init=False, repr=False, default=None) | ||
| bounding_unit: Any = dataclasses.field(init=False, repr=False, default=None) | ||
| bounding_x: Any = dataclasses.field(init=False, repr=False, default=None) | ||
| bounding_y: Any = dataclasses.field(init=False, repr=False, default=None) |
There was a problem hiding this comment.
Many fields in the Block class have been changed from SimpleNamespace or Optional[SimpleNamespace] to Any. While this might resolve immediate mypy errors, it bypasses the benefits of type checking for these attributes. It is recommended to use more specific types or maintain the Optional[SimpleNamespace] annotation where appropriate to ensure long-term maintainability.
There was a problem hiding this comment.
This comment is OBE. Much like the above comment:
Locally Gemini and I have gone round and round today trying to find the ideal collection of types so that mypy passes and so that unittests pass.
What we have now is not necessarily ideal, but it is likely better than what we had and further churn may not be cost effective.
| blocks: List[Block] = [] | ||
| ens = _get_target_object(objects, entities) | ||
| for i in ens: | ||
| for i in cast(Any, ens): |
There was a problem hiding this comment.
The cast(Any, ens) is redundant here because ens is already typed as Any (the return type of _get_target_object). You can iterate over ens directly without the cast.
| for i in cast(Any, ens): | |
| for i in ens: |
… as return type for _get_target_object
…rt _get_target_object return type
…ion.py by narrowing types
2 participants
This PR enables the mypy session in noxfile.py for documentai-toolbox and aligns it with the GAPIC generator template. Currently, it fails with 46 errors. This is pushed to allow farming out the work to resolve these errors or for separate review.