Fix return type annotations for functions used with @contextmanager #8617
Conversation
Some of them are annotated with an `Iterator` return type. However... It just occurred to me that `@contextmanager` cannot work with a function that returns a plain iterator, since it relies on the generator class's `throw` method. `contextmanager` is defined in typeshed as accepting an iterator-returning function, but that appears to be a bug: <python/typeshed#2772>. Change all such annotations to a `Generator` type instead. Some annotations are also broken in other ways; fix them too.
WalkthroughThe changes encompass modifications to type hinting across several Python files, specifically altering return types from Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ProgressReporter
participant Utils
participant MediaExtractor
participant TestUtil
Client->>Client: organization_context() -> Generator[None, None, None]
ProgressReporter->>ProgressReporter: task() -> Generator[None, None, None]
Utils->>Utils: atomic_writer() -> Generator[IO, None, None]
MediaExtractor->>MediaExtractor: read_av_container() -> Generator[av.container.InputContainer, None, None]
TestUtil->>TestUtil: https_reverse_proxy() -> Generator[str, None, None]
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
cvat-sdk/cvat_sdk/core/utils.py (1)
46-46: LGTM: Return type correctly changed to Generator for context manager.
The change from Iterator[IO] to Generator[IO, None, None] is correct because:
- It properly specifies the types for a context manager (yield type, send type, return type)
- It aligns with Python's typing guidelines for
@contextmanager-decorated functions - It matches the actual implementation which yields an IO object
For future reference, when using @contextmanager, always use Generator[YieldType, None, None] as the return type annotation, as context managers don't support send or return operations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- cvat-sdk/cvat_sdk/core/client.py (2 hunks)
- cvat-sdk/cvat_sdk/core/progress.py (2 hunks)
- cvat-sdk/cvat_sdk/core/utils.py (2 hunks)
- cvat/apps/engine/media_extractors.py (1 hunks)
- tests/python/cli/util.py (2 hunks)
🔇 Additional comments (9)
cvat-sdk/cvat_sdk/core/utils.py (1)
16-16: LGTM: Import of Generator type is correctly added.
The addition of Generator import from typing module is necessary and properly placed with other typing imports.
tests/python/cli/util.py (2)
12-12: LGTM: Import statement updated correctly.
The addition of Generator to the imports is necessary for the new type annotation.
42-42: LGTM: Return type annotation correctly updated for @contextmanager usage.
The change from Iterator[str] to Generator[str, None, None] is correct because:
@contextmanagerrequires a generator function that can handle.throw()method calls- The type parameters accurately reflect the function's behavior:
str: the yielded HTTPS URL- First
None: send type (not used in context managers) - Second
None: return type (context managers always return None)
cvat-sdk/cvat_sdk/core/progress.py (2)
9-9: LGTM: Appropriate import addition for Generator type.
The addition of Generator to the typing imports is correct and necessary for the updated return type annotation.
29-29: LGTM: Correct return type for @contextmanager function.
The change from ContextManager[None] to Generator[None, None, None] is correct. The Generator type more accurately represents the implementation requirements of @contextmanager, which needs access to the generator's throw() method. The type parameters [None, None, None] correctly specify that the generator:
- yields
None - doesn't receive values via
send() - doesn't return a value
cvat-sdk/cvat_sdk/core/client.py (3)
13-13: LGTM: Import change aligns with type annotation updates.
The addition of Generator to the imports is necessary to support the updated return type annotations for context manager functions.
124-124: LGTM: Correct return type for context manager.
The change from Iterator[None] to Generator[None, None, None] is correct for @contextmanager decorated functions. The Generator type more accurately represents the context manager protocol by specifying:
- Yield type (None): What the context manager yields
- Send type (None): No values are sent to the generator
- Return type (None): No value is returned after cleanup
This change improves type safety as Generator provides the complete type information needed by the context manager protocol.
124-124: Verify consistent usage of Generator type across context managers.
Let's ensure this type annotation change has been consistently applied across all context manager functions in the codebase.
✅ Verification successful
Based on the search results, I can generate the final response now:
Type annotations for context managers are consistently using Generator
The codebase shows consistent usage of Generator type hints for context manager functions. All context managers found are properly typed:
make_sdk_client() -> Generator[Client, None, None]in testsorganization_context() -> Generator[None, None, None]in cvat-sdk- Other context managers are either test utilities or internal implementation details
The remaining Iterator type hints found in the codebase are used appropriately for different iterator implementations and are not related to context managers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other @contextmanager decorated functions to verify consistent type annotations
echo "Checking for other context manager functions..."
rg --type py "@contextmanager" -B 2 -A 2
# Search for Iterator type annotations that might need to be updated
echo "Checking for remaining Iterator type annotations..."
rg --type py "Iterator\[" -B 2 -A 2
Length of output: 12162
cvat/apps/engine/media_extractors.py (1)
542-544: LGTM! The return type annotation change is correct.
The change from ContextManager to Generator is appropriate here since:
- The function is decorated with
@contextmanager - The function yields a value (the container)
- The type parameters
Generator[av.container.InputContainer, None, None]correctly specify:- The yielded type:
av.container.InputContainer - No send type:
None - No return type:
None
- The yielded type:
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8617 +/- ##
===========================================
+ Coverage 74.22% 74.30% +0.08%
===========================================
Files 403 401 -2
Lines 43375 43319 -56
Branches 3925 3925
===========================================
- Hits 32195 32189 -6
+ Misses 11180 11130 -50
|
Motivation and context
Some of them are annotated with an
Iteratorreturn type. However...It just occurred to me that
@contextmanagercannot work with a function that returns a plain iterator, since it relies on the generator class'sthrowmethod.contextmanageris defined in typeshed as accepting an iterator-returning function, but that appears to be a bug: python/typeshed#2772.Change all such annotations to a
Generatortype instead.Some annotations are also broken in other ways; fix them too.
How has this been tested?
Checklist
developbranch[ ] I have created a changelog fragment[ ] I have updated the documentation accordingly[ ] I have added tests to cover my changes[ ] I have linked related issues (see GitHub docs)[ ] I have increased versions of npm packages if it is necessary(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
New Features
Bug Fixes
_AvVideoReadingclass to address potential memory corruption issues with theavlibrary, providing guidance for developers.Documentation