-
Notifications
You must be signed in to change notification settings - Fork 0
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
tickets/DM-40289: Add support for extra annotations on pod #148
Conversation
9b398a3
to
9cfbd8a
Compare
There's some commentary about why we need to ignore typing for two tests. Strangely, I can't reproduce the bug with a minimal class that does the same thing, so it's at least a little subtle. |
Here's the test case which fails to fail like jupyterlab-controller does. First, the requirements.txt for the virtualenv you should run it in:
And here's
But this behaves exactly as you'd expect:
|
Type narrowing on object attributes in mypy is always a challenge. mypy does want to narrow the discovered type, since otherwise it will get other types of false positives, but it doesn't know whether calling a method could change the value of the attribute, and both decisions can cause problems. They've been struggling with finding the right heuristics for a while. I'm guessing that your test case works because it's all in one file, so mypy has more global knowledge of what the methods do. See python/mypy#11969 for example. I worked around this mypy limitation and updated the dependencies in the pending neophile PR so that I could merge it, so you should be able to drop all of the changes to the test suite and rebase on main now. |
Oh, even more likely than it being all in one file, your example isn't a dataclass, which I suspect changes the way mypy thinks about attributes. (Classes like dataclasses and Pydantic models are more likely to be immutable, and thus calls to apparently unrelated methods are less likely to change the attribute values than a regular class.) |
Oh, and sorry, that was ambiguous: by drop changes to the test suite, I just meant the ones working around the mypy limitation, not the new tests you added (which are fine). |
0e0b524
to
553cdd1
Compare
requirements/dev.in
Outdated
@@ -12,6 +12,7 @@ asgi-lifespan | |||
coverage[toml] | |||
httpx-sse | |||
mypy | |||
# mypy==1.3 |
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.
This can now be removed.
tests/models/domain/rspimage_test.py
Outdated
@@ -155,6 +155,7 @@ def test_collection() -> None: | |||
SystemRandom().shuffle(shuffled_images) | |||
assert images[0].aliases == set() | |||
collection = RSPImageCollection(shuffled_images) | |||
# Same mypy error as above in test_resolve_alias |
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 think this comment is no longer relevant.
Needed for T&S and their bridged-networking magic.