Feature/implement mypy type checking #394
Conversation
|
Thanks for this! I want to poke around with it a little, and then will probably have some questions. The separate GH workflow for mypy is fine for getting things going, but we'll eventually want to move running mypy into tox.ini to simplify local runs. (I can do that later if you're not familiar with tox.) Is there a reason docs needs to be turned into a module? (We deliberately don't support importing from docs.) |
|
Sounds good @medmunds. No need to make docs a module, I adjusted how we're excluding it and removed the |
There was a problem hiding this comment.
I've been playing around with these changes and mypy, and really like where this is going. Thanks again for getting it started.
I have a handful of comments and questions, inline in the code. (GitHub sometimes collapses review comments; search this page for "hidden conversations" to be sure.)
| from email.utils import quote as rfc822_quote | ||
|
|
||
| from requests.structures import CaseInsensitiveDict | ||
|
|
||
| from ..exceptions import AnymailConfigurationError, AnymailWarning | ||
| from ..message import AnymailRecipientStatus | ||
| from ..utils import BASIC_NUMERIC_TYPES, Mapping, get_anymail_setting, update_deep | ||
| from ..utils import BASIC_NUMERIC_TYPES, get_anymail_setting, update_deep |
| @@ -297,7 +296,9 @@ def set_send_at(self, send_at: datetime | str) -> None: | |||
| # "Date and time in the format “YYYY-MM-DD hh:mm:ss” in the UTC time zone." | |||
| # If send_at is a datetime, it's guaranteed to be aware, but maybe not UTC. | |||
| # Convert to UTC, then strip tzinfo to avoid isoformat "+00:00" at end. | |||
| send_at_utc = send_at.astimezone(timezone.utc).replace(tzinfo=None) | |||
| send_at_utc = send_at.astimezone( # type:ignore[union-attr] | |||
There was a problem hiding this comment.
Another option here would be to replace the try/catch with if isinstance(send_at, datetime).
Question: Would you suggest first adding the type:ignore comments without changing code, and then going back to resolve each of them in later commits that change the code?
There was a problem hiding this comment.
If there's an easy fix I'd recommend doing it now - I just didn't see the fix offhand and lean towards keeping MyPy from slowing things down vs. trying to figure out a type hint I can't grok right away, especially if I know the code is fine.
| @@ -0,0 +1 @@ | |||
| AnymailRecipientsType = list[dict[str, dict[str, str]]] | |||
There was a problem hiding this comment.
Is this necessary? (It doesn't seem to be referenced anywhere.)
| disallow_subclassing_any = False | ||
| disallow_untyped_calls = False | ||
| disallow_untyped_defs = False | ||
| ignore_missing_imports = True |
There was a problem hiding this comment.
In my experimenting, I was able to switch ignore_missing_imports back to False by:
pip install -e '.[amazon-ses,resend,postal]'in the workflow, to pick up all the optional dependencies (see earlier comment)- Adding
boto3-stubs[s3,ses,sns]to requirements-dev.txt to pick up boto3 typing (see later comment) - Adding some type:ignore comments on the
_LazyImportErrors in webhooks/amazon_ses.py and webhooks/resend.py (like the ones you already added to webhooks/postal.py).
| pre-commit | ||
| requests |
There was a problem hiding this comment.
requests should not be needed here, since it's already a package dependency. (But you need to pip install -e . to pick up package dependencies, per earlier comment.)
However, I did need to add boto3-stubs[s3,ses,sns] since boto3 isn't typed.
Question: How do library packages typically handle dependencies only needed for typing? Would it be helpful for django-anymail to declare a "typing" extra (pip install django-anymail[typing]) that adds the dependencies on django-stubs and boto3-stubs? Or do libraries usually assume that users will figure out on their own what typing stubs are required?
There was a problem hiding this comment.
Hmmm, for me this was just for my local environment. I generally expect to pip install -r requirements-dev.txt and my local machine would have the necessary packages to run a command like mypy ....that's what led me to make this change.
| @@ -49,7 +49,7 @@ class SparkPostBackendMockAPITestCase(RequestsBackendMockAPITestCase): | |||
| def setUp(self): | |||
| super().setUp() | |||
| # Simple message useful for many tests | |||
| self.message = mail.EmailMultiAlternatives( | |||
| self.message = AnymailMessage( | |||
There was a problem hiding this comment.
This change is no longer required (since you disabled mypy on tests).
But it raises a couple of questions: A lot of the tests use Anymail's added attributes directly on Django's EmailMessage or EmailMultiAlternatives, because this is a documented Anymail feature. (Anymail has always allowed duck typing for its added attributes.)
- Is there some special way we should be testing this works with mypy? (Other than just a test on EmailMessage or EmailMultiAlternatives that sets all of Anymail's attributes with type:ignore comments?)
- Is there some special way Anymail should be exposing this capability for library users? (Like an
AnymailMessageProtocol?)
Co-authored-by: Mike Edmunds <[email protected]>
Co-authored-by: Mike Edmunds <[email protected]>
|
Thank you for the review @medmunds !!! I wanted to respond to your thoughtful comments and also let you know I'm a little busy over the coming months until the new year so won't be able to get back to this...feel free to take over or I will come back to it later! |
This PR adds initial MyPy type checks per discussion in #393.