check typing with mypy#458
Conversation
114836d to
0578047
Compare
terencehonles
left a comment
There was a problem hiding this comment.
Some comments about the changes I've made
| super().init_payload() | ||
| # late-bind in finalize_payload: | ||
| self.recipients = {"to": [], "cc": [], "bcc": []} | ||
| self.recipients: dict[str, list[Any]] = {"to": [], "cc": [], "bcc": []} |
There was a problem hiding this comment.
I'm leaving narrowing Any to the appropriate type as a follow up PR, since I wanted mypy to pass without disabling too many strictness flags, but I didn't want to figure out what was the most narrow type definition would be.
| esp=self.esp_name.lower(), | ||
| version=__version__, | ||
| orig=session.headers.get("User-Agent", ""), | ||
| orig=cast(str, session.headers.get("User-Agent", "")), |
There was a problem hiding this comment.
I assume the UA is generally a string, the alternative would be to use a byte string and never decode the existing UA and just encode all the other parameters since bytes doesn't have a .format but it can interpolate.
| http_headers["Content-Type"] = "application/json" | ||
|
|
||
| super().__init__( | ||
| super().__init__( # type: ignore[misc] |
There was a problem hiding this comment.
I just squelched this error, but technically since *args is unbounded headers can come as a positional argument or a keyword argument. Moving RequestsPayload to use keyword only arguments will fix this issue, but is a breaking change.
There was a problem hiding this comment.
All of the payloads can be considered internal types.
fwiw, in other projects I'm starting to drop *args and **kwargs where they're not absolutely necessary. But maybe start with this change, then improve the API as a follow-on commit.
(btw, this is likely to be part of a breaking change release anyway. We might just add a blanket "internals have been reworked" changelog item.)
| return HttpResponse() | ||
|
|
||
| def parse_json_body(self, request: HttpRequest) -> dict | list | None: | ||
| def parse_json_body(self, request: HttpRequest) -> dict: |
There was a problem hiding this comment.
This looks like it was a bad type definition because below it was only used as a dict and would break if it were a list or None.
| content_type = self.get_content_type() | ||
| content = self.get_content_bytes() | ||
| return SimpleUploadedFile(name, content, content_type) | ||
| return SimpleUploadedFile(name, content, content_type) # type: ignore[arg-type] |
There was a problem hiding this comment.
name may be None here, but I don't know if the Django stubs type definition is wrong by requiring non None, if get_filename() might actually not be none, or if this code needs to change.
| ) | ||
| # headersonly forces an empty string payload, which breaks things later: | ||
| msg.set_payload(None) | ||
| msg.set_payload(None) # type: ignore[call-overload] |
There was a problem hiding this comment.
The Python typeshed seems to be incorrect if this is valid, but I didn't look into the source to see what happens here, but I'm going to assume this is valid.
There was a problem hiding this comment.
There were definitely some significant errors in the Python typeshed around mail last year (when I was looking at this for Django), particularly in legacy APIs like set_payload. I'm not sure the current status.
There was a problem hiding this comment.
It looks like this is threading a needle which does work according to the source, but it's unlikely a PR will be accepted to change this. Looking into this more should this be calling the clear_content method instead? That will strip some headers, but I assume any content-* headers aren't valid without content.
| BASIC_NUMERIC_TYPES = (int, float) | ||
|
|
||
|
|
||
| UNSET = type("UNSET", (object,), {}) # Used as non-None default value |
There was a problem hiding this comment.
This is basically a funny way to write class UNSET: pass, which the latter mypy can actually use since it's a concrete type, but this is used as a singleton which mypy doesn't like for is checks so I've replaced this implementation with an implementation I use for the same functionality including the type that mypy should use.
There was a problem hiding this comment.
It looks like this needs to be pickle safe, so I've used the TYPE_CHECKING definition which is more like the original definition where there is a class defined, but this still creates a singleton of that class.
There was a problem hiding this comment.
I mean, the right thing to do here is a Sentinel, but that keeps getting deferred.
| content = self.content | ||
| if isinstance(content, str): | ||
| content = content.encode(self.charset) | ||
| content = content.encode(self.charset or settings.DEFAULT_CHARSET) |
There was a problem hiding this comment.
charset seems like it's only guaranteed to be non None if it's a "text/*" mime type, I assume this is coming from the user, so if for some reason the charset isn't defined the default makes sense, right?
|
|
||
|
|
||
| class CaseInsensitiveCasePreservingDict(CaseInsensitiveDict): | ||
| _store: OrderedDict |
There was a problem hiding this comment.
typeshed doesn't define _store (trying to keep it private)
4773003 to
1127487
Compare
|
@medmunds friendly ping (just making sure you also saw this PR) |
Co-authored-by: Terence D. Honles <terence@honles.com>
Co-authored-by: Terence D. Honles <terence@honles.com>
|
Thanks for picking this up! (And of course thanks to @YPCrumble for the earlier work.) I won't have time to get into details before mid-next-week at the earliest. In the meantime:
|
|
I figured we might just target |
This change adds tox configuration to run mypy, and updates the type information so test passes. --- Co-Authored-By: Ian Campbell <ipcampbell@gmail.com>
1127487 to
ef330c6
Compare
This picks up stalled PR #394. I'm not sure if squashed PRs are the default, but it makes sense to squash all this work to get rid of the earlier churn, but keep the co-authorship.