Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider type hints for user inputs #393

Open
YPCrumble opened this issue Aug 22, 2024 · 5 comments
Open

Consider type hints for user inputs #393

YPCrumble opened this issue Aug 22, 2024 · 5 comments
Labels
feature/enhancement future design Ideas/improvements that need some design work

Comments

@YPCrumble
Copy link

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 the esp_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:

# Old (now incorrect implementation:
esp_extra = {"transactional": True, ...}

# New (now correct) implementation
esp_extra = {"options": {"transactional": True, ...}, ...}

The issue I faced was that transactional and other settings as part of esp_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 my AnymailMessage.

@medmunds
Copy link
Contributor

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.

@medmunds medmunds added feature/enhancement future design Ideas/improvements that need some design work labels Aug 22, 2024
@YPCrumble
Copy link
Author

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.

@YPCrumble
Copy link
Author

@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.

@medmunds
Copy link
Contributor

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.

@YPCrumble
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/enhancement future design Ideas/improvements that need some design work
Projects
None yet
Development

No branches or pull requests

2 participants