-
-
Notifications
You must be signed in to change notification settings - Fork 33.3k
gh-137840: Implement PEP 728 (closed and extra_items in typing.TypedDict) #137933
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
Changes from 16 commits
cbea6f8
4cde2f9
06de7a9
407bf74
f9755d3
385812a
bdbae49
e68c2ed
8203bc1
1b3ce52
2f5c42e
1bd3a6d
42a1aac
36ee4d2
503b825
1f23caa
9a9c5c5
18b929b
611b3e6
a6250f4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| :class:`typing.TypedDict` now supports the ``closed`` and ``extra_items`` | ||
| keyword arguments (as described in :pep:`728`) to control whether additional | ||
| non-required keys are allowed and to specify their value type. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -123,6 +123,63 @@ PyTypeObject _PyNoDefault_Type = { | |
|
|
||
| PyObject _Py_NoDefaultStruct = _PyObject_HEAD_INIT(&_PyNoDefault_Type); | ||
|
|
||
| /* NoExtraItems: a marker object for TypedDict extra_items when it's unset. */ | ||
|
||
|
|
||
| static PyObject * | ||
| NoExtraItems_repr(PyObject *op) | ||
| { | ||
| return PyUnicode_FromString("typing.NoExtraItems"); | ||
| } | ||
|
|
||
| static PyObject * | ||
| NoExtraItems_reduce(PyObject *op, PyObject *Py_UNUSED(ignored)) | ||
| { | ||
| return PyUnicode_FromString("NoExtraItems"); | ||
| } | ||
|
|
||
| static PyMethodDef noextraitems_methods[] = { | ||
| {"__reduce__", NoExtraItems_reduce, METH_NOARGS, NULL}, | ||
| {NULL, NULL} | ||
| }; | ||
|
|
||
| static PyObject * | ||
| noextraitems_new(PyTypeObject *type, PyObject *args, PyObject *kwargs) | ||
| { | ||
| if (PyTuple_GET_SIZE(args) || (kwargs && PyDict_GET_SIZE(kwargs))) { | ||
| PyErr_SetString(PyExc_TypeError, "NoExtraItemsType takes no arguments"); | ||
| return NULL; | ||
| } | ||
| return (PyObject *)&_Py_NoExtraItemsStruct; | ||
| } | ||
|
|
||
| static void | ||
| noextraitems_dealloc(PyObject *obj) | ||
| { | ||
| /* This should never get called, but we also don't want to SEGV if | ||
| * we accidentally decref NoExtraItems out of existence. Instead, | ||
| * since NoExtraItems is an immortal object, re-set the reference count. | ||
| */ | ||
| _Py_SetImmortal(obj); | ||
| } | ||
|
|
||
| PyDoc_STRVAR(noextraitems_doc, | ||
| "NoExtraItemsType()\n" | ||
| "--\n\n" | ||
| "The type of the NoExtraItems singleton."); | ||
|
|
||
| PyTypeObject _PyNoExtraItems_Type = { | ||
ZeroIntensity marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| PyVarObject_HEAD_INIT(&PyType_Type, 0) | ||
| "NoExtraItemsType", | ||
| .tp_dealloc = noextraitems_dealloc, | ||
| .tp_repr = NoExtraItems_repr, | ||
| .tp_flags = Py_TPFLAGS_DEFAULT, | ||
| .tp_doc = noextraitems_doc, | ||
| .tp_methods = noextraitems_methods, | ||
| .tp_new = noextraitems_new, | ||
ZeroIntensity marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| }; | ||
|
|
||
| PyObject _Py_NoExtraItemsStruct = _PyObject_HEAD_INIT(&_PyNoExtraItems_Type); | ||
|
|
||
| typedef struct { | ||
| PyObject_HEAD | ||
| PyObject *value; | ||
|
|
||
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.
Is
rangethe builtin here?That's a bit strange.
Uh oh!
There was an error while loading. Please reload this page.
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 agree it looks a bit odd as an arbitrary
extra_itemsvalue, but it's valid, since it just means extra keys must have values assignable to range. For context, this test is copied fromtyping_extensions, where it was used to cover the same case. We could edit it for readability, but I kind of like it as a reminder for maintainers thatextra_itemsaccepts any type, not just the obvious ones.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.
That could be valid for a regular test that validates stored values.
this however is a negative test, isn’t it?
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.
It's still valid/correct as a negative test. By "valid" do you mean "correct" or "readable?" Passing range into extra_items is valid, and passing closed=True at the same time should raise a runtime error.
Uh oh!
There was an error while loading. Please reload this page.
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.
Sorry I was being terse.
I'm trying to say that this test:
Could have an
extra_items=rangecounterpart.That would make a solid test, both understandable and useful.
Meanwhile, the negative test,
class TD(TypedDict, closed=True, extra_items=range): --> errorwould be better served with a simpler, more straightforwardextra_items=intargument.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.
P.S. my comment overall is minor, please don't let me stop your work!
Uh oh!
There was an error while loading. Please reload this page.
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.
Thanks for clarifying. I thought about it some more.
I don't think introducing an
extra_items=rangecounterpart would actually widen the test coverage, since it wouldn't be exercising any new behavior. PEP 728 splits responsibilities between runtime and type checker behavior. Whileextra_itemsis only supposed to accept a valid type expression, validating that is the type checker's job (e.g. MyPy's valid-type error), not the runtime's. The runtime just stores whatever is passed in.So the real subject under test is simply:
We don't need multiple values to prove that behavior.
And although
rangeis a less obvious type, I think it makes sense in the negative test. That test is specifically asserting the error message “Cannot combineclosed=Trueandextra_items”. Usingrangehighlights that the failure comes from the combination, not fromrangeitself being an invalid value ofextra_items.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 tend to use
rangesometimes as it's a builtin type that isn't generic (likelist) and doesn't participate in promotion weirdness (likefloatand historicallybytes), so it's a good basic type to test with.Plus, people who forget that
rangeis a type get to learn something :)