Finalize typing improvements for spf.py and bimi.by #226
Conversation
…ct annotations in spf.py Co-authored-by: seanthegeek <44679+seanthegeek@users.noreply.github.com>
Co-authored-by: seanthegeek <44679+seanthegeek@users.noreply.github.com>
Co-authored-by: seanthegeek <44679+seanthegeek@users.noreply.github.com>
There isn't a way to make a TypeDict field optional until Python 3.11, and we need to support 3.9. Leaving the typedicts in place for now, so that they can be used when Python 3.10 is no longer suported.
There was a problem hiding this comment.
Pull request overview
This pull request finalizes typing improvements for spf.py and bimi.py, replacing OrderedDict with standard dict types, adding comprehensive TypedDict definitions for structured data, and improving type annotations throughout. The changes modernize the codebase to use Python 3.9+ type hints and make the code more maintainable with explicit type definitions.
Key changes:
- Replaced
OrderedDictwith regulardictand addedTypedDictdefinitions for structured return types in spf.py - Added comprehensive
TypedDictdefinitions for BIMI record structures in bimi.py - Updated function signatures to use
Sequence[str | Nameserver]for nameserver parameters and removed unnecessaryOptionalwrappers around primitive types with defaults - Minor formatting improvements in other files (line breaks, blank lines, trailing whitespace)
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| checkdmarc/spf.py | Major refactoring replacing OrderedDict with TypedDict-based structures, updated type annotations, and improved mechanism parsing with explicit type definitions |
| checkdmarc/bimi.py | Added TypedDict definitions for BIMI structures (SVGMetadata, CertificateMetadata, BIMIQueryResult, etc.) and updated function return types |
| checkdmarc/smtp_tls_reporting.py | Formatting change to split long class definition across multiple lines |
| checkdmarc/mta_sts.py | Formatting improvements (line splits and trailing whitespace removal) |
| checkdmarc/dmarc.py | Added blank lines after TypedDict docstrings for consistency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
checkdmarc/spf.py
Outdated
| else: | ||
| warnings.append( | ||
| "Any text after the all mechanism other than an exp modifier is ignored." | ||
| ) |
There was a problem hiding this comment.
The else clause is incorrectly indented and will execute when the try block succeeds without exception, not when an exception occurs. This should be dedented to be at the same level as the if statement on line 639, so it only executes when items_after_all doesn't start with "exp=".
| else: | |
| warnings.append( | |
| "Any text after the all mechanism other than an exp modifier is ignored." | |
| ) | |
| else: | |
| warnings.append( | |
| "Any text after the all mechanism other than an exp modifier is ignored." | |
| ) |
checkdmarc/spf.py
Outdated
| warnings.append("No TXT records at exp value {exp}.") | ||
| if len(exp_txt_records) > 1: | ||
| warnings.append("Too many TXT records at exp value {exp}.") |
There was a problem hiding this comment.
The f-string formatting is missing. The string should be an f-string to interpolate the exp variable: f"No TXT records at exp value {exp}."
| warnings.append("No TXT records at exp value {exp}.") | |
| if len(exp_txt_records) > 1: | |
| warnings.append("Too many TXT records at exp value {exp}.") | |
| warnings.append(f"No TXT records at exp value {exp}.") | |
| if len(exp_txt_records) > 1: | |
| warnings.append(f"Too many TXT records at exp value {exp}.") |
checkdmarc/spf.py
Outdated
| warnings.append("No TXT records at exp value {exp}.") | ||
| if len(exp_txt_records) > 1: | ||
| warnings.append("Too many TXT records at exp value {exp}.") |
There was a problem hiding this comment.
The f-string formatting is missing. The string should be an f-string to interpolate the exp variable: f"Too many TXT records at exp value {exp}."
| warnings.append("No TXT records at exp value {exp}.") | |
| if len(exp_txt_records) > 1: | |
| warnings.append("Too many TXT records at exp value {exp}.") | |
| warnings.append(f"No TXT records at exp value {exp}.") | |
| if len(exp_txt_records) > 1: | |
| warnings.append(f"Too many TXT records at exp value {exp}.") |
| a_records = get_a_records( | ||
| value, | ||
| nameservers=nameservers, | ||
| resolver=resolver, | ||
| timeout=timeout, | ||
| timeout_retries=timeout_retries, | ||
| ) |
There was a problem hiding this comment.
The ptr mechanism logic calls get_a_records(value, ...) but doesn't handle the case where value is an empty string. Unlike the "a" and "mx" mechanisms which default to the current domain when value is empty, the ptr mechanism doesn't check for this. This could cause an error if value is empty.
checkdmarc/bimi.py
Outdated
| # TypedDict definitions for BIMI record structures | ||
|
|
||
|
|
||
| # These typedicts can't be used in Python 3.9-3.10 because these is no way to set a field as optional |
There was a problem hiding this comment.
The comment contains a grammatical error. It should say "there is no way" instead of "these is no way".
| # These typedicts can't be used in Python 3.9-3.10 because these is no way to set a field as optional | |
| # These typedicts can't be used in Python 3.9-3.10 because there is no way to set a field as optional |
| mx_host_addresses = {} | ||
| for host in mx_hosts: | ||
| hostname = host["hostname"] | ||
| # --- perform A/AAAA resolution for each MX host --- | ||
| try: | ||
| _addresses = get_a_records( | ||
| hostname, | ||
| nameservers=nameservers, | ||
| resolver=resolver, | ||
| timeout=timeout, | ||
| timeout_retries=timeout_retries, | ||
| ) | ||
| mx_host_addresses[hostname] = _addresses |
There was a problem hiding this comment.
The variable mx_host_addresses is populated but never used. The MX mechanism stores mx_hosts (which contains just the MX records) instead of storing the resolved addresses. This appears to be dead code that should either be removed or the mechanism should store the resolved addresses.
| """Raised when a duplicate SPF include is found""" | ||
|
|
||
|
|
||
| class SPFRecordNotFound(SPFError): |
There was a problem hiding this comment.
This class does not call SPFError.init during initialization. (SPFRecordNotFound.init may be missing a call to a base class init)
| other_spf_dns_mechanism: SPFDNSLookupMechanism = { | ||
| "action": action, | ||
| "mechanism": mechanism, | ||
| "value": value, | ||
| "dns_lookups": mechanism_dns_lookups, | ||
| "void_dns_lookups": mechanism_void_dns_lookups, | ||
| } |
There was a problem hiding this comment.
This statement is unreachable.
No description provided.