-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Upgrade mypy #12293
Upgrade mypy #12293
Changes from all commits
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 |
---|---|---|
|
@@ -37,8 +37,9 @@ def overridden() -> None: | |
) | ||
old() | ||
|
||
# https://github.com/python/mypy/issues/2427 | ||
self.configuration._load_config_files = overridden # type: ignore[assignment] | ||
self.configuration._load_config_files = ( # type: ignore[method-assign] | ||
overridden | ||
) | ||
Comment on lines
+40
to
+42
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we just global-ignore |
||
|
||
@contextlib.contextmanager | ||
def tmpfile(self, contents: str) -> Iterator[str]: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we should make FakeDistribution not abstract instead? We can just raise in all methods that are not supposed to be called. |
||
assert dist.direct_url is None | ||
mock_read_text.assert_called_once_with(DIRECT_URL_METADATA_NAME) | ||
|
||
|
@@ -35,7 +35,7 @@ def test_dist_get_direct_url_invalid_json( | |
class FakeDistribution(BaseDistribution): | ||
canonical_name = cast(NormalizedName, "whatever") # Needed for error logging. | ||
|
||
dist = FakeDistribution() | ||
dist = FakeDistribution() # type: ignore[abstract] | ||
with caplog.at_level(logging.WARNING): | ||
assert dist.direct_url is None | ||
|
||
|
@@ -84,7 +84,7 @@ def test_dist_get_direct_url_valid_metadata(mock_read_text: mock.Mock) -> None: | |
class FakeDistribution(BaseDistribution): | ||
pass | ||
|
||
dist = FakeDistribution() | ||
dist = FakeDistribution() # type: ignore[abstract] | ||
direct_url = dist.direct_url | ||
assert direct_url is not None | ||
mock_read_text.assert_called_once_with(DIRECT_URL_METADATA_NAME) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, why does this warn about the abstract class? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. mypy is correct to complain here. If you do 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". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. … 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. |
||
|
||
with pytest.raises(NoneMetadataError) as exc: | ||
_check_dist_requires_python( | ||
|
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.
What changed here? This seems to be ignored in a lot of places.
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'm guessing something now thinks
get
can return None, because it can't infer from the earlierif 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.