Improve Type Safety and fix mypy errors #137
Conversation
* Replace TYPE_CHECKING conditions with direct type variable declarations (zero overhead in runtime) * Add TypeGuard for wrapped function checks * Add Protocol for location providers * Implement proper type casting for decorator return types * Add typing_extensions as a project dependency * Remove redundant ctx parameters from AST nodes Signed-off-by: Templin Konstantin <[email protected]>
|
@orsinium, hello! Please check changes |
| @@ -61,7 +60,7 @@ def pre( | |||
| message=message, | |||
| exception=exception or _exceptions.PreContractError, | |||
| ) | |||
| func = partial(Contracts.attach, 'pres', contract) | |||
| func = cast(Callable[[C], C], partial(Contracts.attach, 'pres', contract)) | |||
There was a problem hiding this comment.
This type casting serves no purpose. cast doesn't check the validity of the cast. Also, mypy now can correctly type check partial calls, so cast even reduces the type safety.
There was a problem hiding this comment.
The reason for creating these changes is mypy errors, including CI (job example: https://github.com/life4/deal/actions/runs/11742383633/job/32712993184).
You say that mypy already knows how to handle partial correctly? Then to resolve these errors, we can make a bump version of mypy?
| from typing import Callable, Iterator, TypeVar | ||
| from typing import Callable, Iterator, Protocol, TypeVar | ||
|
|
||
| from typing_extensions import TypeGuard |
There was a problem hiding this comment.
with __future__ import, typing_extensions imports can be placed behind if TYPE_CHECKING, making the dependency optional.
| @@ -52,6 +62,6 @@ def get_contracts(func: Callable) -> Iterator[Contract]: | |||
| if contracts.patcher: | |||
| yield _wrappers.Has(contracts.patcher) | |||
|
|
|||
| if not hasattr(func, '__wrapped__'): | |||
| if not has_wrapped(func): | |||
There was a problem hiding this comment.
Not much value in this, tbh: the __wrapped__ is used the very next line. While I like mypy with all my heart, type safety makes more sense when it brings checks not only to the very next line.
There was a problem hiding this comment.
Okay, got it! Maybe then add # type: ignore on this line?
|
Thank you for your contribution. While I appreciate the effort you've put into it, I'm not concerned too much about internal type safety: deal is very stable and extensively tested, including mutational testing and dog-fooding. However, I'm always open to improvements, including type safety, to the API consumed by the library users. So, don't hesitate if you ever have any of those. |
Thanks for your feedback. Initially, I wanted to contribute to improving contracts (I have a couple of ideas). But when running tests locally, I got a lot of errors on mypy check, so I decided to fix them. So the main goal of the current Pull Request is to fix mypy errors. |
However, I would like to discuss some of my ideas, so creating a pull request right away is not the best idea, because maintainers might reject this request. I don't see an issues section in the repository. How can we discuss the changes? |
|
Thank you, that's good to know. I think the problem is that this PR does too many things at once. Let's split the effort. Please, bring a PR with your ast improvements, I'd be happy to merge it. And I'll have a closer look how to make mypy happy. |
Description
Type System Improvements
__wrapped__attributeNew
has_locationfunction to verify AST node attributesWithWrappedSpecialfor wrapped function handlingLocationProviderfor AST node location informationImplemented proper return type casting for decorators using
typing.castRemoved redundant
ctx=ast.Load()parameters from AST node constructionAdded missing level=0 parameter to
ImportFromnodesSimplified
ASTnode creation where context was unnecessaryDependencies
Added
typing_extensions>=4.12.2as a required dependency to support new typing featuresTesting