♻️ Maintenance: mypy webserver part 2 #6142
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6142 +/- ##
=========================================
- Coverage 84.5% 82.0% -2.6%
=========================================
Files 10 577 +567
Lines 214 29422 +29208
Branches 25 234 +209
=========================================
+ Hits 181 24136 +23955
- Misses 23 5226 +5203
- Partials 10 60 +50
Flags with carried forward coverage won't be shown. Click here to find out more.
|
GitHK
requested review from
pcrespov,
sanderegg and
matusdrobuliak66
as code owners
|
| confirmation_token | ||
| ).data | ||
| validated = _InvitationValidator.parse_obj(confirmation_token) | ||
| invitation_data: InvitationData = validated.data # type: ignore[assignment] |
There was a problem hiding this comment.
actually you should not have to do that. Json[InvitationData] is of type InvitationData as it is a wrapper. The problem is that you enforce the type here and mypy rightfully complains.
| async with self.pool.acquire() as conn: | ||
| return await _sql.find_one(conn, self.user_tbl, with_data) | ||
| result: asyncpg.Record | None = await _sql.find_one( |
There was a problem hiding this comment.
when you put a type like this, you actually force it to be so. normally that is not needed. and both pylance and mypy shall know it is asyncpg.Record | None.
There was a problem hiding this comment.
Also you are now changing the syntax of that function. what happens if the user is None? is it handled?
| async with self.pool.acquire() as conn: | ||
| await _sql.update(conn, self.user_tbl, {"id": user["id"]}, updates) | ||
|
|
||
| async def delete_user(self, user): | ||
| async def delete_user(self, user) -> None: |
There was a problem hiding this comment.
same for delete_user and update_user
| ProductPriceInfo | None, | ||
| await repo.get_product_latest_price_info_or_none(current_product_name), | ||
| ) |
There was a problem hiding this comment.
why is this cast needed here?? this repo is in the code right? this should be defined as the return type is it not?
| @@ -91,7 +95,9 @@ async def get_product_stripe_info( | |||
| app: web.Application, *, product_name: ProductName | |||
| ) -> ProductStripeInfoGet: | |||
| repo = ProductRepository.create_from_app(app) | |||
| product_stripe_info = await repo.get_product_stripe_info(product_name) | |||
| product_stripe_info: ProductStripeInfoGet = await repo.get_product_stripe_info( | |||
There was a problem hiding this comment.
why if not product_stipe_info? if this is the type then this is wrong.
| @@ -48,7 +48,7 @@ async def auto_create_products_groups(app: web.Application) -> None: | |||
|
|
|||
| async with engine.acquire() as connection: | |||
| async for row in iter_products(connection): | |||
| product_name = row.name | |||
| product_name = row.name # type: ignore[attr-defined] | |||
There was a problem hiding this comment.
why you ignore it here over casting?
| lrange = redis_client.lrange( | ||
| get_notification_key(user_id), -1 * MAX_NOTIFICATIONS_FOR_USER_TO_SHOW, -1 | ||
| ) | ||
| raw_notifications: list[str] = ( | ||
| await lrange if isinstance(lrange, Awaitable) else lrange | ||
| ) |
There was a problem hiding this comment.
we use it async so we know it is an awaitable right? why use the if here and not just cast or ignore?
| all_user_notifications: list[UserNotification] = [ | ||
| UserNotification.parse_raw(x) for x in await redis_client.lrange(key, 0, -1) | ||
| UserNotification.parse_raw(x) | ||
| for x in (await lrange if isinstance(lrange, Awaitable) else lrange) |
| ] | ||
| for k, user_notification in enumerate(all_user_notifications): | ||
| if req_path_params.notification_id == user_notification.id: | ||
| user_notification.read = body.read | ||
| await redis_client.lset(key, k, user_notification.json()) | ||
| lset = redis_client.lset(key, k, user_notification.json()) | ||
| if isinstance(lset, Awaitable): |
| @@ -71,8 +71,10 @@ def _parse_extra_credits_in_usd_or_none( | |||
| confirmation: ConfirmationTokenDict, | |||
| ) -> PositiveInt | None: | |||
| with suppress(ValidationError, JSONDecodeError): | |||
| invitation = InvitationData.parse_raw(confirmation.get("data", "EMPTY")) | |||
| return invitation.extra_credits_in_usd | |||
| confirmation_data: str | None = confirmation.get("data", "EMPTY") | |||
There was a problem hiding this comment.
This is now more convoluted. The idea here is to raise ValudationErrorif something goes wrong and then suppressed.
if confirmation_data is None, then parse_raw will raise. There is no need for this check
|
|
||
| repo = await self.ReposOrm(conn).set_filter(id=repo_id).fetch("project_uuid") | ||
| assert repo # nosec | ||
| return repo.project_uuid | ||
| return ProjectIDStr(repo.project_uuid) |
There was a problem hiding this comment.
NOTE that using the constractor or a constrainted str type simply calls the constructor of str but does NOT run any validation. For that you need to do parse_obj_as(ProjectIDStr, repo.project_uuid)
There was a problem hiding this comment.
normally this comes with sqlalchemy[mypy]
| @@ -78,14 +78,14 @@ async def load_products_on_startup(app: web.Application): | |||
| async with engine.acquire() as connection: | |||
| async for row in iter_products(connection): | |||
| try: | |||
| name = row.name | |||
| name = row.name # type: ignore[attr-defined] | |||
There was a problem hiding this comment.
Do you know why mypy cannot access the attributes of row? Could it be that we are missing here another mypy stub?
|
this will be taken care of in different PRs |
What do these changes do?
Remaining:
Found 164 errors in 52 files (checked 388 source files)The following modules have been touched:
Related issue/s
How to test
Dev-ops checklist