Upgrade mypy #12293
Upgrade mypy #12293
Conversation
Mostly straightforward. The new message.get_all type ignores out of fear it may return None are minorly annoying. You can see some of the extra errors on the pip codebase in the mypy_primer of the typeshed PR that made the change python/typeshed#9620 The abstract errors are because FakeDistribution doesn't actually implement any of the Protocol it claims to implement
| value = sanitise_header(msg.get(field)) | ||
| value = sanitise_header(msg.get(field)) # type: ignore[arg-type] |
There was a problem hiding this comment.
What changed here? This seems to be ignored in a lot of places.
There was a problem hiding this comment.
I'm guessing something now thinks get can return None, because it can't infer from the earlier if field not in msg that the field is guaranteed to be present.
I think this sort of ignore needs a more extensive comment, as it feels very much like a workaround for a limitation in mypy's ability to infer types correctly, or a lack of expressiveness in the type system to annotate get in such a way that this sort of LBYL idiom is supported.
We could of course add an assert stating that the return from get is not None, but I think that sort of assert is counterproductive, and obscures the actual logic here.
There was a problem hiding this comment.
Yup, I mentioned this in the body of the PR: python/typeshed#9620
I can add a comment, should I do so in all affected places or only here?
There was a problem hiding this comment.
I don't honestly know. This is the sort of "typing is frustrating" issue I've been going on about on Discourse. I dislike #type: ignore because it normalises switching off typing without documenting why. After all, someone could remove the check and then we're ignoring a genuine error.
I guess we should add a comment like "field is guaranteed to be present (see check on line XXX)". Unfortunately, the #type: ignore syntax doesn't appear to allow explanatory trailing text, which would be preferable.
At the end of the day, I'm just expressing my frustration. I don't really mind how we address this, it's not like we're worse off than we would be without type annotations at all.
There was a problem hiding this comment.
For this particular one I’d just add an explanation comment here. Other occurrences are in tests and those are less problematic especially a reader can more easily find the comment by tracing the test code.
| @@ -23,7 +23,7 @@ def test_dist_get_direct_url_no_metadata(mock_read_text: mock.Mock) -> None: | |||
| class FakeDistribution(BaseDistribution): | |||
| pass | |||
|
|
|||
| dist = FakeDistribution() | |||
| dist = FakeDistribution() # type: ignore[abstract] | |||
There was a problem hiding this comment.
Maybe we should make FakeDistribution not abstract instead? We can just raise in all methods that are not supposed to be called.
| self.configuration._load_config_files = ( # type: ignore[method-assign] | ||
| overridden | ||
| ) |
There was a problem hiding this comment.
Should we just global-ignore method-assign in all files in tests.
| @@ -252,7 +252,7 @@ class NotWorkingFakeDist(FakeDist): | |||
| def metadata(self) -> email.message.Message: | |||
| raise FileNotFoundError(metadata_name) | |||
|
|
|||
| dist = make_fake_dist(klass=NotWorkingFakeDist) | |||
| dist = make_fake_dist(klass=NotWorkingFakeDist) # type: ignore[type-abstract] | |||
There was a problem hiding this comment.
Hmm, why does this warn about the abstract class?
There was a problem hiding this comment.
Similar to the above, NotWorkingFakeDist doesn't implement BaseDistribution. So make_fake_dist gives you an object that doesn't do what it says it should
There was a problem hiding this comment.
For my understanding, is there an actual runtime problem here, or are we simply not able to express what we're doing in terms that the type system can understand?
It feels like we shouldn't be trying to type check mock objects like this as if they are full-fledged versions. After all, that's the whole point of mock objects, surely? What do we hope to gain from the type annotations in this case?
There was a problem hiding this comment.
mypy is correct to complain here. If you do dist.from_directory or many of the other methods defined by the BaseDistribution protocol, you'll see that dist does not implement the Protocol it claims to implement. You could use a narrower Protocol to type _check_dist_requires_python, but I think you'd find that not worth it.
So instead we just say "get out of my way, I know I'm doing a thing that could be unsafe, but I don't care because it's a test". # type: ignore is as good a way to say this as any other, such as casting to Any. In general, type checking tests is lower value than type checking other code.
There was a problem hiding this comment.
… Continuing from the comment above, it should be OK to add a comment here (and one for FakeDistribution) to say we’re kind ot intentionally not fleshing out the implementation since an unimplemented abstract method would correctly raise and make a test fail if it’s accidentally called.
|
Apologies, I missed there was an existing PR when I created #12389 I think this one can be closed now. |
Mostly straightforward.
The new message.get_all type ignores out of fear it may return None are minorly annoying. You can see some of the extra errors on the pip codebase in the mypy_primer of the typeshed PR that made the change python/typeshed#9620
The abstract errors are because FakeDistribution doesn't actually implement any of the Protocol it claims to implement