- Notifications
You must be signed in to change notification settings - Fork263
typeddicts - update conformance tests according to the spec#1978
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| # > A key that is not a literal should generally be rejected. | ||
| movie: Movie = {variable_key: "", "year": 1900} # E: variable key | ||
| # Destructive operations. | ||
| existing_movie[variable_key] = 1982 # E: variable key |
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.
I'd like to get clarification on the wording in the spec before we add these tests. I've started a discussionhere. Feel free to add your viewpoint to the thread.
| # It's not clear from the spec what type this should be. | ||
| movie.get("name") | ||
| # It's not clear from the spec what type this should be. | ||
| movie.get("other") # E? | ||
| # It's not clear from the spec whether it's allowed. | ||
| reveal_type("other" in movie) # E? |
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.
In general, we should not add tests to the conformance suite in areas where the spec is not clear. If you think that clarity is needed (and is beneficial), then please start a discussion about amending the spec.
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.
I think I've overlooked the spec a bit - I've made assumption thatin with unknown key is ambigious similar tomovie.get("other") # E? but now I've found that spec actually states that all operations with unknown key should be rejected:
The use of a key that is not known to exist should be reported as an error, even if this wouldn’t necessarily generate a runtime type error.
So I assume it's the same situation as with arbitrary strings - spec prohibits it, but in reality
- setitem, getitem, del - rejected by everyone (mypy, Pyre, pyright)
.pop("other")- rejected by Pyre and mypy.get("other")- only rejected by Pyre"other" in movieis not rejected by any type checker
Can you please take a look if I'm not missing something? And I'll start a similar discussion.
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.
The spec doesn't provide any specific guidance for how type checkers should synthesize methods on TypedDicts (get,pop, etc.). Unless you think there's value in standardizing this behavior, we should leave the spec and the conformance tests as is. If you think there's value in standardizing the behavior here (I'm somewhat skeptical that it's worth the effort), then we should amend the spec and update the conformance tests accordingly.
get should never be rejected because it's safe to call (i.e. doesn't raise an exception) for a not-present key. It even has a form that accepts a default value that is returned when the key isn't present.
Thein operator should also never be rejected because it never raises an exception.
My preference is to leave the conformance test as it is currently and abandon this PR. I think the current test reflects what is in the spec.
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.
I think there is a value in clarifying the spec. Currently it says that use of unknown keys should be reported as an error
but the spec is interpreted by each type checker in a bit different way. I've started a discussion to gather opinions from the community -https://discuss.python.org/t/typeddict-operations-with-unknown-literal-keys/89653
d155152 to459f25bCompare
Added missing tests to typeddicts_operations.py based on the current state ofhttps://typing.python.org/en/latest/spec/typeddict.html#supported-and-unsupported-operations
Stumbled upon type checkers having a different behaviour for
typed_dict[str], investigated the spec and the spec is clear about type checkers generally rejecting operations with arbitrary strings - so wanted to reflect the spec in conformance tests.Added tests for:
popitem.getand within- spec is not certain about this, so I guess it allows both behaviours..getandin.Added baseline output commit just for a reference - to confirm that there no other typecheckers version changes at play in the updated output. I can remove it before merge to avoid additional diffs.