-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat: enable mypy session for ndb #16691
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
base: main
Are you sure you want to change the base?
Changes from 4 commits
990f5a9
d4f5310
9934515
ec2312c
a316b6f
51c6e18
787e904
4f9468d
0ee10cc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import datetime | ||
| import re | ||
| import time | ||
| from typing import Any | ||
|
|
||
| from google.cloud.ndb import context as context_module | ||
| from google.cloud.ndb import exceptions | ||
|
|
@@ -485,7 +486,7 @@ def _Literal(self): | |
| a string, integer, floating point value, boolean or None). | ||
| """ | ||
|
|
||
| literal = None | ||
| literal: Any = None | ||
|
|
||
| if self._next_symbol < len(self._symbols): | ||
| try: | ||
|
|
@@ -770,27 +771,34 @@ def _raise_cast_error(message): | |
|
|
||
|
|
||
| def _time_function(values): | ||
| t_tuple: tuple[int, ...] | ||
| if len(values) == 1: | ||
| value = values[0] | ||
| if isinstance(value, str): | ||
| try: | ||
| time_tuple = time.strptime(value, "%H:%M:%S") | ||
| st = time.strptime(value, "%H:%M:%S") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replace
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gemini suggested this refactor to clean up this code and reduce the number of if statements
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These two code blocks are fundamentally the same with the exception of item In your comment we assert that Gemini recommends this refactor to reduce the number of Funnily enough: Those Here is Gemini's rationale for the approach taken in this PR (this was the third change we made and Gemini felt it was the most important): < BEGINNING of GEMINI's exposition >
This is the most important change for Mypy: Old: Mypy cannot guarantee at compile-time that the length of the tuple matches what the function expects. It sees New: if len(t_tuple) == 1:
return datetime.time(t_tuple[0])
elif len(t_tuple) == 2:
return datetime.time(t_tuple[0], t_tuple[1])
# ...The Benefit: This is called Exhaustive Checking or Manual Unrolling. < END of GEMINI's exposition > I am happy to resolve this in whatever way feels best to you, but I suspect that if I revert back to using the splat operator, we will have to go through another cycle of trying to find a way to keep mypy happy OR add one or more ignore pragmas. |
||
| except ValueError as error: | ||
| _raise_cast_error( | ||
| "Error during time conversion, {}, {}".format(error, values) | ||
| ) | ||
| time_tuple = time_tuple[3:] | ||
| time_tuple = time_tuple[0:3] | ||
| t_tuple = (st.tm_hour, st.tm_min, st.tm_sec) | ||
| elif isinstance(value, int): | ||
| time_tuple = (value,) | ||
| t_tuple = (value,) | ||
| else: | ||
| _raise_cast_error("Invalid argument for time(), {}".format(value)) | ||
| elif len(values) < 4: | ||
| time_tuple = tuple(values) | ||
| t_tuple = tuple(values) | ||
| else: | ||
| _raise_cast_error("Too many arguments for time(), {}".format(values)) | ||
| try: | ||
| return datetime.time(*time_tuple) | ||
| if len(t_tuple) == 1: | ||
| return datetime.time(t_tuple[0]) | ||
| elif len(t_tuple) == 2: | ||
| return datetime.time(t_tuple[0], t_tuple[1]) | ||
| elif len(t_tuple) == 3: | ||
| return datetime.time(t_tuple[0], t_tuple[1], t_tuple[2]) | ||
| else: | ||
| _raise_cast_error("Invalid arguments for time()") | ||
| except ValueError as error: | ||
| _raise_cast_error("Error during time conversion, {}, {}".format(error, values)) | ||
|
|
||
|
|
||
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.
Did you mean to have
assertas part of this code path? Usually this is only used in testsUh oh!
There was an error while loading. Please reload this page.
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.
I looked for ways to provide type narrowing for mypy and
assertis one way it can be done.It is admittedly controversial for some in production code.
I went ahead and switched the
asserts intoifclauses throughout.