Fix issubclass() to narrow down types of type variables#7930
Fix issubclass() to narrow down types of type variables#7930ilevkivskyi merged 8 commits intopython:masterfrom TH3CHARLie:fix-issubclass
issubclass() to narrow down types of type variables#7930Conversation
ilevkivskyi
left a comment
ilevkivskyi
left a comment
There was a problem hiding this comment.
Thanks for PR! Here I have few comments.
|
Thanks for the detailed review! I patched it up in df62cbc
|
ilevkivskyi
left a comment
ilevkivskyi
left a comment
There was a problem hiding this comment.
Thanks for updates! Here are some more comments.
| if len(node.args) != 2: # the error will be reported elsewhere | ||
| return {}, {} | ||
| expr = node.args[0] | ||
| if literal(expr) == LITERAL_TYPE: |
There was a problem hiding this comment.
I would keep the above two lines in this function. To make the refactoring more 1-to-1.
|
|
||
| def infer_issubclass_maps(self, node: CallExpr, | ||
| type_map: Dict[Expression, Type] | ||
| ) -> Tuple[TypeMap, TypeMap]: |
There was a problem hiding this comment.
Please add a short docstring for the new function.
| return other.field | ||
| return 0 | ||
|
|
||
| [builtins fixtures/isinstancelist.pyi] |
There was a problem hiding this comment.
OK, now the two classes don't look the same, but you didn't do what I asked, I wanted type variables with different upper bounds, like Type[SomeClass], not just type.
ilevkivskyi
left a comment
ilevkivskyi
left a comment
There was a problem hiding this comment.
Some more comments here. Also please don't hurry, it is not a competition.
|
You are right and I shouldn't be in such a hurry. I had some misunderstanding about this issue from the very beginning and that mistake continues in my testcase. I apologize for that and am trying my best to fill my knowledge gap on type systems(mainly by reading TAPL) Now I look it again and realize |
|
Thanks for the guidance! Again, quite an education on both software engineering and type system aspects and that means a lot to me. |
2 participants
resolves #7920
Currently, no testcases break so I did not look into
meet.pyfor further investigation