Conversation
This comment has been minimized.
This comment has been minimized.
A5rocks
left a comment
A5rocks
left a comment
There was a problem hiding this comment.
I assume you accidentally committed a bunch of files? The lxml changes and the std_types changes are not necessary and shouldn't be in this PR. (and changing our vendored typeshed is a very bad idea)
I also would prefer another implementation.
docs/source/getting_started.rst
Outdated
| .. note:: | ||
|
|
||
| When adding types, the convention is to import types | ||
| When adding types, the convention is to import types as std_types |
There was a problem hiding this comment.
This is talking about importing typing and this isn't the convention.
There was a problem hiding this comment.
Yeah, the convention is "never alias-import stdlib modules unless strictly necessary". Please revert this doc change along with all imports garbled in other files.
| # and isinstance(ct.arg_types[0], TypeVarType) | ||
| and ct.arg_types[0] == ct.ret_type | ||
| ): | ||
| return None |
There was a problem hiding this comment.
This is looks like a special case for this decorator:
def f(x: T) -> T:
return xyou should add a special case in addition to this.
There was a problem hiding this comment.
Well, technically this line is limiting to that special case. And this will even fix the linked issue, two callables of that shape will compare equal AFAIC.
But this also makes the following (completely wrong) deco generate a signature:
def deco(fn: str) -> str: ...
@deco
def foo():
print()So I agree there should be one more special case, not just blanket "one-arg function returning its arg unchanged".
There was a problem hiding this comment.
I would argue that any identity function should be fine here -- if that decorator happens to have the wrong types that's not really this code's problem (it's just trying to unwrap the underlying callable) and I suspect there's many many ways to type an identity function with varying specificity making this particular condition difficult-to-impossible to write correctly exhaustively
| # and isinstance(ct.arg_types[0], TypeVarType) | ||
| and ct.arg_types[0] == ct.ret_type | ||
| ): | ||
| return None |
There was a problem hiding this comment.
Well, technically this line is limiting to that special case. And this will even fix the linked issue, two callables of that shape will compare equal AFAIC.
But this also makes the following (completely wrong) deco generate a signature:
def deco(fn: str) -> str: ...
@deco
def foo():
print()So I agree there should be one more special case, not just blanket "one-arg function returning its arg unchanged".
docs/source/getting_started.rst
Outdated
| .. note:: | ||
|
|
||
| When adding types, the convention is to import types | ||
| When adding types, the convention is to import types as std_types |
There was a problem hiding this comment.
Yeah, the convention is "never alias-import stdlib modules unless strictly necessary". Please revert this doc change along with all imports garbled in other files.
test-data/unit/stubgen.test
Outdated
|
|
||
| [case testCoroutineImportTypesAsT] | ||
| import types as t | ||
| import types as std_types as t |
There was a problem hiding this comment.
sed is great, but this isn't even valid python.
This comment has been minimized.
This comment has been minimized.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
|
Ok, I reverted the std_types changes, but I am still having trouble getting my Daemon to run. After talking to my instructor, I have decided I will no longer work on this issue, as it will not meet the requirements for my classwork assignment. |
4 participants
Fixes #18940 (hopefully; I wrote a test, but I'm having trouble running it)
How this PR changes mypy:
Deleted the line mentioned in the original issue, added an import called "types", and made a few other small changes.
I think there's a problem with the test I wrote. For some reason, when I type dmypy check ., it says Daemon has died. For the sake of my team's academic assignment, I'd like to label this as a work in progress tentatively, then return to it later because I can't let anyone else claim it for now.