Mypy app-wide scope #786
Mypy app-wide scope #786
Conversation
why-not-try-calmer
added
the
chore
why-not-try-calmer
requested review from
suricactus and
faebebin
and removed request for
suricactus
| if isinstance(some_user, User): | ||
| users.append(some_user) | ||
| else: | ||
| users += some_user | ||
| assert len( |
There was a problem hiding this comment.
could it be:
users: list[User] = [some_user] if isintance(some_user, User) else some_users
BTW i would rather keep the var name users because it is simpler?
There was a problem hiding this comment.
Yes that works too, that's actually a good use of variable shadowing
Thanks, ready for me |
There was a problem hiding this comment.
Hey, @why-not-try-calmer @faebebin . Thanks for the contribution and reviews.
There are a couple of problems that from this PR, the worst of them in docker-app/qfieldcloud/subscription/models.py line 815 -> the assert there is changing the behaviour of the app, therefore breaking it.
I caught this error when trying to integrate this PR changes with web UI.
I would suggest for such quite fundamental changes we have a double review, cool down period and opening a PR on the web ui to verify the changes are working.
See #791 for the fixes.
| 'Organization "{}" cannot be added. Only users and teams can be collaborators.' | ||
| ).format(username), | ||
| message = _( | ||
| f'Organization "{username}" cannot be added. Only users and teams can be collaborators.' |
There was a problem hiding this comment.
I have already commented this in another place, this replacing "{}".format(1) with f"{1}" is causing troubles for potential translations.
| @@ -794,7 +794,7 @@ def create_subscription( | |||
| plan: Plan, | |||
| created_by: Person, | |||
| active_since: Optional[datetime] = None, | |||
| ) -> Tuple["Subscription", "Subscription"]: | |||
| ) -> tuple[Optional["AbstractSubscription"], "AbstractSubscription"]: | |||
There was a problem hiding this comment.
Why don't you use the definition from the docs: tuple[AbstractSubscription | None, AbstractSubscription] ?
| @@ -794,7 +794,7 @@ def create_subscription( | |||
| plan: Plan, | |||
| created_by: Person, | |||
| active_since: Optional[datetime] = None, | |||
There was a problem hiding this comment.
This one should be datetime | None as all other places, no?
| assert active_since | ||
|
|
There was a problem hiding this comment.
This assert is actually breaking the system, as we have situations where we DO allow no active since to be passed. You can clearly see that from the typing of the function argument.
I merged this quite early after only 1 approval so it's on me. I think it makes sense indeed to let you / the project owner do the merges for anything non-trivial, especially when several pending PRs can impact each other. |
This extends the scope of the static type-checker to also encompass docker-app and docker-redis.