Repository navigation
Declare missing dependencies - #290
Conversation
97df4d8 to
90e7cac
Compare
90e7cac to
31404de
Compare
|
The minimum package versions for attrs and typing-extensions are copied from Synapse: The minimum package version for pyopenssl is copied from aioapns: |
| "mypy==0.812", | ||
| "mypy-zope==0.3.0", | ||
| "tox", | ||
| "mypy==0.812", |
There was a problem hiding this comment.
I'd put mypy-zope after mypy. But perhaps mypy- comes before mypy=?
Fine as it is, just an aside.
There was a problem hiding this comment.
Good spot, I'll fix it.
| "attrs>=19.2.0", | ||
| "cryptography>=2.6.1", | ||
| "idna>=2.8", | ||
| "importlib_metadata", |
There was a problem hiding this comment.
Do we want/have a lower bound here?
There was a problem hiding this comment.
Similarly to comment on typing-extensions, should we declare this as importlib_metadata; python_version < "3.8" (assuming we don't need the new importlib.metadata bits from 3.10)
There was a problem hiding this comment.
Perhaps. I have no idea what the lower bound should be
There was a problem hiding this comment.
For reference, this is what we do in python-signedjson: matrix-org/python-signedjson#9
|
Another thought: why did CI miss this? I guess there's no equivalent of the |
I think we're not testing against older python versions (e.g., I think setting up a matrix of |
| "sentry-sdk>=0.10.2", | ||
| "service_identity>=18.1.0", | ||
| "Twisted>=19.7", | ||
| "typing-extensions>=3.7.4", |
There was a problem hiding this comment.
Should we make this conditional so it's only installed on Py < 3.8? (And do the same for the import that needs it)
There was a problem hiding this comment.
I think we typically import directly from typing_extensions; would have to change the source to do a try...except ImportError... dance
There was a problem hiding this comment.
On the import side, mypy does not appreciate the obvious attempt:
https://mypy-play.net/?mypy=latest&python=3.10&gist=9ac99f23480e5c8bc86c68f94ce65b6c
if TYPE_CHECKING or sys.version_info < (3, 8, 0):
from typing_extensions import Literal
else:
from typing import Literalworks with mypy but always requires typing_extensions in dev.
There was a problem hiding this comment.
There's a PR to do the same for python-signedjson here: matrix-org/python-signedjson#12
Possibly because it was brought in by a dev dependency (e.g. |
3.7, the one we test with, is the oldest version we support iirc. |
DMRobertson
left a comment
DMRobertson
left a comment
There was a problem hiding this comment.
I'm happy if CI is happy. Sanity check: would this have fixed the earlier deployment to matrix.org?
It ought to, assuming we do a |
Maybe a |
A On another note, mypy really does not like conditional imports. Getting the right combination of Python 3.7: Python 3.8, importlib_metadata installed: Python 3.8, importlib_metadata not installed: |
Ouch, thanks for working through that. Is there any way that we could use |
I can't see how, unless we unconditionally use importlib_metadata when type checking. and you've made me realise that the dependencies are wrong. typing_extensions is only installed on Python <3.8, but we use it in type checking mode regardless. |
|
mypy brings in typing-extensions for us, but we should be explicit. |
Ahhh, I was more getting at "why did the deployment fail?" so we could check this fixes it. The cause was an inability to import typing_extensions, but that's now been made a hard requirement. Edit: a hard requirement on sufficiently old Pythons. So I think this is hopefully good to go! 👍 |
4 participants
It's best to look at the commits separately.
The first commit sorts the list of dependencies.
Fixes #289