-
Notifications
You must be signed in to change notification settings - Fork 402
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(mypy): complete mypy support for the entire codebase #943
feat(mypy): complete mypy support for the entire codebase #943
Conversation
@heitorlessa github ci failing on the last error: https://github.com/mploski/aws-lambda-powertools-python/runs/4759867427?check_suite_focus=true. I’d like to discuss a potential solution to this. In general mypy complains on subclass overwriting superclass get method with different signatures, which I believe breaks Liskov Substitution Principle. Other errors were solved already. |
Since any change to this method's spec may be breaking to users and we cannot also move get method declaration in Base class to subclass for the same reason decided to silent it and accept potential risk |
55b177b
to
bbb18b4
Compare
Since any change to this method spec may be breaking to users and we cannot also move get method declaration in Base class we need to silent it and accept potential risk
630441e
to
8d23b2d
Compare
Codecov Report
@@ Coverage Diff @@
## develop #943 +/- ##
========================================
Coverage 99.96% 99.96%
========================================
Files 119 119
Lines 5320 5337 +17
Branches 613 608 -5
========================================
+ Hits 5318 5335 +17
Partials 2 2
Continue to review full report at Codecov.
|
8d23b2d
to
be81184
Compare
be81184
to
8bcb3fd
Compare
5d57d5a
to
0196883
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @mploski - it covers a lot of ground and tech debt.
I'd like to ask you to consider two key aspects:
- Consider
cast
your last resource.cast
is gonna increase the execution time for end-customers, and it slowly litters the code (similar to type coercion in other languages). Sometimes propagating the type, a generic, or creating another variable and an explicit type is a solution. - Consider documenting beyond typing changes. I've noticed that we seem to be introducing a new bug in Idempotency by fixing a type, as we're changing a branching logic too. There are major block changes in Parameters too. For quick notes, use the PR description, and for larger ones like parameters comment on why this is necessary. Mypy can't see beyond the types, and while we have multiple checks in place we might slip a regression here. By changing that code, it'd be great to check whether additional tests are necessary too as we might have not covered that.
aws_lambda_powertools/utilities/idempotency/persistence/base.py
Outdated
Show resolved
Hide resolved
aws_lambda_powertools/utilities/idempotency/persistence/dynamodb.py
Outdated
Show resolved
Hide resolved
284bde2
to
0196883
Compare
66f2a0c
to
862c67e
Compare
20ce752
to
a82e959
Compare
@heitorlessa Added few tests to fix tests coverage |
Added the last bits - thank you so so much again @mploski ;) A new era for Powertools. We must also keep a close eye after the release to see if this accidentally introduced any new bug that our checks + peer review wasn't able to detect. |
Issue #, if available:
aws-powertools/powertools-lambda#3
Description of changes:
Checklist
Breaking change checklist
RFC issue #:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.