Open YPCrumble opened 3 months ago
Thanks for the kind words, and sorry you got tripped up by a breaking change.
I'd really like to get type annotations into Anymail. I started looking into this a few years ago, but had trouble getting useful feedback from mypy and django-stubs at the time. (There might have been a problem using django-stubs with a library project that doesn't have a Django settings file, but my memory is hazy. I think both packages have improved quite a bit since then, so it's perhaps time to try again.)
If you have experience with getting type checking working in Django projects (and it sounds like you do), I'd welcome the help. A reasonable first step might be something like getting mypy running in the project and then annotating AnymailMessageMixin?
Typing esp_extra
is going to be tricky: the valid values depend on which ESP you're using. At best it would be a union of several TypedDicts (which I suspect makes for a pretty confusing type error message). And I'd be a little concerned about release churn trying to track every ESP's API parameters.
But also, esp_extra
is meant to allow use of newly-introduced ESP API features without having to wait for a new Anymail release, so its correct type is really probably dict[str, Any]
(which wouldn't have helped you). I'm open to ideas here.
Thanks @medmunds - I'm happy to contribute! Just made an initial PR for enabling type hinting and fixed a few issues. There are more improvements to be made in the future but this is a good start.
On type hinting esp_extra - I have one initial question, which is how is set_esp_extra
actually used? Or is this a legacy method that should be removed? I can't see it in use anywhere but defined across many models.
I don't think type hinting esp_extra
would cause much release churn - if a new setting were in use for an ESP in the codebase MyPy would flag it and we'd simply add the setting to the TypedDict.
@medmunds your comment about esp_extra
as an "escape hatch" makes sense to me. I also think that having type hints would make people likely to contribute back to this repo with those configuration options, which would help the community. Otherwise they'd need to use # type: ignore
on those lines in their code.
how is
set_esp_extra
actually used?
All of the *Payload.set_*
methods are called from BasePayload.__init__
, through some code that involves more magic than I'd like. (I'm actually planning a pretty major refactoring to get rid of the separate payload classes, to try to make adding new ESPs more straightforward. So I wouldn't worry too much about typing the payload internals right now.)
type hinting
esp_extra
…
My concern is less about uses of esp_extra
inside Anymail, and more about uses in other people's code. Right now, if ESPs add new sending API features every week, Anymail can happily ignore them (for features outside Anymail's "normalized sending"), knowing that developers can still use the new features through esp_extra
. But if it's strongly typed, the typings will frequently be out of date. That's my concern about churn.
Do you know if Python typing supports the equivalent of TypeScript's "interface merging," where developers can redeclare an interface locally to add new fields? If so, something like that might be the easier way to handle esp_extra
typing.
Aha! I thought there was some magic going on somewhere. That's definitely hard for a reader to grok, but I suppose I would have started searching for set_
had I noticed that none of the set_x methods were being called directly anywhere.
I don't think Python has that functionality yet but something similar, a TypedDict with optional extra keys, might be available starting in Python 3.13: https://peps.python.org/pep-0728/
I also think users with type hints are pretty used to typing.cast
or # type: ignore[some-rule]
lines in the codebase if they're waiting on a package upgrade. But I'm also not as aware as you of how frequently the APIs change for the various backends.
Thank you for maintaining this repo! It's been a fantastic resource for my sites.
I recently did a major upgrade and was bitten by a breaking change in v8, whereby SparkPost's
esp_extra
needed various settings to be nested inside of"options"
instead of at the top level of theesp_extra
dict. The issue was that this failed silently and I didn't read the changelog closely enough or pay close enough attention to the SparkPost esp_extra documentation.Simple example:
The issue I faced was that
transactional
and other settings as part ofesp_extra
were just ignored.I'd love to implement type hints for the SparkPost backend (or other backends if that's helpful) and that would be welcomed by maintainers. My idea is that having a type hint using
typing.TypedDict
would have prevented me from inputting the wrong shape of data into myAnymailMessage
.