-
Notifications
You must be signed in to change notification settings - Fork 129
fix: use broader iterable type #1614
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1614 +/- ##
=======================================
Coverage 92.22% 92.22%
=======================================
Files 55 55
Lines 8411 8414 +3
Branches 973 973
=======================================
+ Hits 7757 7760 +3
Misses 466 466
Partials 188 188 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jsignell
left a comment
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.
As mentioned below, the type changes are good, but I think changing the positional arg names seems unnecessary since it's breaking.
The constructor is an instance method, not a class method, so call the first parameter "self".
The variable is mutable so the type is invariant, use the Iterable type that it is declared as in the superclass.
jsignell
left a comment
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.
LGTM
Related Issue(s):
Description:
Use
selfas name for the first parameter for instance methods, and align type annotations to match the superclass type annotation for mutable variables.PR Checklist:
pre-commit run --all-files)pytest)