-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
chore(issues): Simple type improvements #70697
Conversation
self, group: Group, forecast_values=list[int], date_added=datetime | ||
self, group: Group, forecast_values: list[int], date_added: datetime |
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.
This is the only non-typing change, no actual effect since defaults are never used and they clearly would have been broken if they were.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #70697 +/- ##
==========================================
- Coverage 80.04% 80.03% -0.01%
==========================================
Files 6505 6505
Lines 290739 290742 +3
Branches 50102 50102
==========================================
- Hits 232709 232703 -6
- Misses 57593 57602 +9
Partials 437 437
|
def process_occurrence_group_with_shutdown(*args, **kwargs): | ||
process_occurrence_group(*args, **kwargs) | ||
def process_occurrence_group_with_shutdown(items: list[Mapping[str, Any]]) -> None: | ||
process_occurrence_group(items) |
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.
Feels like only **kwargs
can be replaced by the list[Mapping[str, Any]]
type and *args?
cannot since there's no keywords? Or is there some Python/mypy magic happening behind the scenes that makes this okay?
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.
**kwargs
could have only been passed as items=...
before as the underlying signature of process_occurrence_group
is (list[Mapping[str, Any]]) -> None
-- so this just makes the signature match explicitly
with *args, **kwargs
mypy infers that as "this is a callable that could take any number of positional or named arguments" -- now it's explicitly "one possibly-named argument possibly positional argument named items
of type ..."
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! assuming there's some magic going on in test_run.py
that I'm not aware of which is probably the case
If only mypy could automatically assume a function's return type was None if its not explicit 😢
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.
Another quick batch of followups to #69828.
For review, no cross-file changes so each one is independently reviewable (aside from pyproject of course). Also, there are a handful in here which are not comprehensive - this intent of this PR is just to chip away at some easy improvements even if they're not yet enough to fully opt a module in quite yet.