Open
Conversation
There was a problem hiding this comment.
Pull request overview
This PR focuses on improving type hints around log_decorator and related exports to reduce/avoid mypy ignores and make the decorator easier to type-check across the package.
Changes:
- Adds
ParamSpec/overload-based typing tolog_decorator, and types wrapper arguments. - Removes a mypy ignore on
partial(log_decorator, ...)usage. - Updates public exports / changelog and adds
typing_extensionsdependency.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/risclog/logging/log.py |
Removes type: ignore on partial(log_decorator, ...) now that log_decorator has improved typing. |
src/risclog/logging/decorators.py |
Introduces overloads + ParamSpec typing for log_decorator and wrapper argument typing. |
src/risclog/logging/__init__.py |
Adjusts how log_decorator is re-exported and simplifies type aliases. |
setup.py |
Adds typing_extensions to support ParamSpec on older Python versions. |
CHANGES.rst |
Records the type-hint fix in the unreleased section. |
Comments suppressed due to low confidence (3)
src/risclog/logging/decorators.py:68
- The async branch’s wrapper is annotated as returning
Any, but for anasync definput function the decorated callable’s return type should remain an awaitable of the original awaited result. With the currentCallable[P, R]overload,Rwill be inferred as aCoroutine[...], andasync def wrapper(...) -> Anydoes not match that and loses type information for callers. Consider adding a dedicated overload for async callables (e.g.,Callable[P, Awaitable[T]] -> Callable[P, Awaitable[T]]) and typing the async wrapper to preserveT.
if inspect.iscoroutinefunction(func):
@wraps(func)
async def wrapper(*args: P.args, **kwargs: P.kwargs) -> Any:
if not logging.getLogger(logger.name).isEnabledFor(logging.DEBUG):
src/risclog/logging/decorators.py:192
return cast(Callable[P, R], wrapper)is currently masking a real type mismatch between the sync and async wrapper implementations (especially for coroutine functions). This makes the public type signature look correct while type checkers can’t validate it. After splitting sync/async overloads, return the correctly typed wrapper without a broad cast (or only cast within the specific branch where it’s proven safe).
return cast(Callable[P, R], wrapper)
src/risclog/logging/init.py:4
- Redundant aliasing:
from ... import log_decorator as log_decoratorcan be simplified tofrom ... import log_decoratorfor clarity (the file already uses aliases only when names differ, e.g.,getLogger as logger).
from risclog.logging.decorators import log_decorator as log_decorator
from risclog.logging.log import HybridLogger, get_logger as old_get_logger
💡 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.
No description provided.