-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Implement class syntax for TypedDict #2808
Conversation
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 the PR! Looks good, just some minor things:
mypy/semanal.py
Outdated
if any(not isinstance(expr, RefExpr) or | ||
expr.fullname != 'mypy_extensions.TypedDict' and | ||
not self.is_typeddict(expr) for expr in defn.base_type_exprs): | ||
self.fail("All bases of a new TypedDict must be TypedDict's", defn) |
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.
Grammar nit: Replace TypedDict's
with TypedDict types
or similar.
mypy/semanal.py
Outdated
tpdict = None # type: OrderedDict[str, Type] | ||
for base in typeddict_bases: | ||
# mypy doesn't yet understand predicates like is_typeddict | ||
tpdict = base.node.typeddict_type.items # type: ignore |
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 recommend using a cast instead of # type: ignore
, since it's more specific about where mypy gets confused.
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 required me three casts to convince mypy that everything is OK and it became unreadable, I used asserts instead.
mypy/semanal.py
Outdated
else: | ||
name = stmt.lvalues[0].name | ||
if name in (oldfields or []): | ||
self.fail('Cannot overwrite TypedDict field {} while extending' |
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.
Use double quotes around field name (e.g., ... TypedDict field "x" ...
).
# flags: --python-version 3.6 | ||
from mypy_extensions import TypedDict | ||
|
||
class Point1(TypedDict): |
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.
Add test case for duplicate fields within a single definition. Example:
class Bad(TypedDict):
x: int
x: int
fields.append(name) | ||
types.append(AnyType() if stmt.type is None else self.anal_type(stmt.type)) | ||
# ...despite possible minor failures that allow further analyzis. | ||
if name.startswith('_'): |
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.
Why not allow fields starting with underscores? Existing JSON data may use underscore prefixes, I think.
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 this is probably an unnecessary limitation (I copied it from the functional form, and that one probably copied it from namedtuple
). Maybe it is worth making a separate PR removing this limitation from both forms?
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 sounds reasonable.
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 no-underscore-prefix limitation allows extending the syntax of the functional form with new keywords in the future if it becomes useful.
I believe the no-underscore-prefix limitation for namedtuples used a similar rationale.
@JukkaL Thank you for review! I implemented changes in a new commit. |
Thank you! |
Fixes #2740
This allows the following:
(Side note: I spent an hour figuring out what are anonymous and named TypedDict's, maybe better call them explicit/implicit, or explicit/ihferred, if I understood them correctly?)