Add type annotations to db/util.py and configured mypy #3280
Conversation
skools-here
force-pushed
the
fix-db-types
branch
2 times, most recently
from
8b20725 to
c7734aa
Compare
There was a problem hiding this comment.
seems like an okay first pass (i still need to test it locally)
Overall notes:
- any changes to the actual logic are likely going to slow this down a fair bit as the logic needs testing to make sure it behaves the same
- also, try to avoid using
Anyas a type if you can at all avoid it - its not very helpful at enforcing typings since it accepts anything.
augur/application/db/util.py
Outdated
|
|
||
| # do the sleep here instead of instead of in the exception | ||
| # so it doesn't sleep after the last failed time | ||
| if attempts > 0: | ||
| #Do a 30% exponential backoff | ||
| # Do a 30% exponential backoff |
There was a problem hiding this comment.
this seems like a non-typing change that removed a helpful comment - was this removed on purpose?
augur/application/db/util.py
Outdated
|
|
||
| def convert_orm_list_to_dict_list(result): | ||
| new_list = [] | ||
| def convert_orm_list_to_dict_list(result: List[Any]) -> List[Dict[str, Any]]: |
There was a problem hiding this comment.
it might be nice to not use Any as the type here - does the ORM have a base/parent class that all the ORM classes inherit from? maybe thats a better thing to use?
There was a problem hiding this comment.
I’ll update convert_orm_list_to_dict_list to use the ORM base class, and refine config_dict + logger to use more specific types (Optional[logging.Logger], etc.) .
augur/application/db/util.py
Outdated
|
|
||
| data_type = config_dict["type"] | ||
| def convert_type_of_value( | ||
| config_dict: Dict[str, Any], logger: Optional[Any] = None |
There was a problem hiding this comment.
does config_dict come from somewhere? could its type be specified more precisely than Any? same with the logger
augur/application/db/util.py
Outdated
| if data_type == "str" or data_type is None: | ||
| if data_type is None or data_type == "str": |
There was a problem hiding this comment.
why was the order of these changed? this could cause a crash if the data type is none because it is relying on the short curcuiting behavior of or
There was a problem hiding this comment.
Yes i will fix this this .
| else: | ||
| config_dict["value"] = True | ||
| value = str(config_dict["value"]).lower() | ||
| config_dict["value"] = value != "false" |
There was a problem hiding this comment.
if value is exactly false (string), this will evaluate to true.
There was a problem hiding this comment.
wait did i get this wrong?
There was a problem hiding this comment.
the earlier function (Not type safe one) already had that anything not false is True behavior. So according to me this must work i am not sure though .
|
Yes thanks for the review will be changing all the logic changes which may cause problem and will also try to reduce the use of any . |
skools-here
force-pushed
the
fix-db-types
branch
from
941b7e8 to
7314d01
Compare
|
we should also probably add a CI job to check the types so its clear if something breaks on a pull request |
| "pytest==6.2.5", | ||
| "toml>=0.10.2", | ||
| "ipdb==0.13.9", | ||
| "mypy>=1.8.0", |
There was a problem hiding this comment.
if you change this file, you also need to run uv sync and commit the changes to the uv.lock file. this will likely help some of the CI jobs pass
skools-here
force-pushed
the
fix-db-types
branch
from
8c883ec to
41eb3c9
Compare
Signed-off-by: Sajal-Kulshreshtha <sajalkulshreshtha9@gmail.com>
Signed-off-by: Sajal-Kulshreshtha <sajalkulshreshtha9@gmail.com>
Signed-off-by: Sajal-Kulshreshtha <sajalkulshreshtha9@gmail.com>
Signed-off-by: Sajal-Kulshreshtha <sajalkulshreshtha9@gmail.com>
Signed-off-by: Sajal-Kulshreshtha <sajalkulshreshtha9@gmail.com>
Signed-off-by: Sajal-Kulshreshtha <sajalkulshreshtha9@gmail.com>
skools-here
force-pushed
the
fix-db-types
branch
from
412b13f to
e6126e5
Compare
Description
This PR introduces type annotations to the augur\application\db\util.py file and adds mypy for static type checking.
This PR fixes #3274
Changes included:
Notes for Reviewers
Signed commits