Improve type definition of FlattenableItems for better type narrowing#511
Open
hirosassa wants to merge 2 commits into
Open
Improve type definition of FlattenableItems for better type narrowing#511hirosassa wants to merge 2 commits into
hirosassa wants to merge 2 commits into
Conversation
768f361 to
72fdf1e
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refines the FlattenableItems recursive type alias to use concrete container types (list, tuple, dict) instead of Iterable, enabling modern type checkers (e.g., Pyright) to correctly narrow types after isinstance checks (notably distinguishing dict from other containers).
Changes:
- Updated
FlattenableItemsingokart/utils.pyto uselist/tuple/dictfor improved type narrowing. - Adjusted
map_flattenable_items/ related branches to align runtime/type-checking behavior with the new alias. - Added casts and updated type-handling logic in
task.pyandgcs_obj_metadata_client.pyto satisfy stricter typing and narrowing.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
gokart/utils.py |
Updates FlattenableItems definition and narrows map_flattenable_items container handling. |
gokart/task.py |
Tweaks requires() return typing and adds a cast for required_task_outputs mapping. |
gokart/gcs_obj_metadata_client.py |
Removes Iterable-based narrowing; uses `list |
Comments suppressed due to low confidence (1)
gokart/gcs_obj_metadata_client.py:136
- The TypeError message says "dict, or iterable of RequiredTaskOutput", but this branch now only accepts
list | tuple(anddict). Please update the message to match the supported input types so callers aren't misled when debugging.
elif isinstance(required_task_outputs, list | tuple):
return [GCSObjectMetadataClient._get_serialized_string(ro) for ro in required_task_outputs]
else:
raise TypeError(
f'Unsupported type for required_task_outputs: {type(required_task_outputs)}. '
'It should be RequiredTaskOutput, dict, or iterable of RequiredTaskOutput.'
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What I did
Replace
Iterablewith concrete collection types (list,tuple) in theFlattenableItemsrecursive type alias to enable proper type narrowing afterisinstancechecks.Since dict, list, and tuple are disjoint, type checkers can now correctly narrow the type after isinstance(x, dict), resolving many false positives in Pyright and other modern type checker tools.
Motivation
The original Iterable union prevented type narrowing because dict is a subtype of Iterable, making the branches overlap.
This caused Pyright to report errors on dict indexing, return types, and argument types across task.py, utils.py, and gcs_obj_metadata_client.py.