-
Notifications
You must be signed in to change notification settings - Fork 251
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
Parse raw metadata #671
Parse raw metadata #671
Conversation
Co-authored-by: Donald Stufft <[email protected]>
Can't figure out how to even trigger the situation.
Apparently `List` is invariant, so its types must be **exact**.
Overall, this LGTM. Just a few minor points (mostly typos). And can I just say that I appreciate the extensive commenting of the code, it made it very easy to see what was going on, and getting this right is fiddly, so it's doubly important to have a good understanding of the code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks great here!
Most of those comments are compliments of @dstufft . 🙂 |
Co-authored-by: Donald Stufft <[email protected]> Co-authored-by: Paul Moore <[email protected]>
@pradyunsg what do you want to do about the Python 3.8 requirement? Should I hold off on merging this until 3.7 hits EOL, or at this point are we assuming the next release can drop 3.7 support regardless of when it comes out? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that if you want you can get this to work and type check on 3.7.
Just use:
if typing.TYPE_CHECKING:
from typing_extensions import TypedDict
class RawMetadata(TypedDict, total=False): ...
else:
RawMetadata = Dict[str, Any] # or even just object()
Type checkers are special cased to always know what typing_extensions is, even if it's not installed. If you want something a little less mendacious, you can use an additional sys.version_info check
Let's hold off, for now; unless what's suggested above works. :) |
The current linting error can be fixed by upgrading isort to >= 5.12, see PyCQA/isort#2077 |
The suggestion from @hauntsaninja seems to work! @pradyunsg now what do you think about merging this? |
If you want to make the TypedDict class available at runtime (on 3.8 and newer), and get rid of an indent level, here is what a less mendacious version could look like:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I trust @brettcannon's judgement on what to do with the TypedDict
story, with a preference for not dropping support for 3.7 eagerly. :)
If you're happy with the logic around |
I'm trying out the latest suggestion from @hauntsaninja since it might help something like Pydantic that might care about |
Thank for the help, everyone! |
Part of #570
This PR copies the
RawMetadata
andparse_email()
parts of #574 , adds tests and docs, and makes everything to work. I'm purposefully ignoring any emission to other formats or parsing of anything but email headers to make progress.Because this PR uses
TypedDict
and we have no dependencies, it can't be supported by Python 3.7. Luckily it hits EOL in June of this year. But that does mean we need to wait to merge this until we are ready to drop Python 3.7 support (which I'm personally ready to do 😁).