Add a bunch of type annotations to a bunch of modules#293
Conversation
baseplate/__init__.py
Outdated
| import raven | ||
|
|
||
|
|
||
| def metrics_client_from_config(raw_config: Dict[str, str]) -> metrics.Client: |
There was a problem hiding this comment.
The funding instrument service saw this Dict[str, str] show up in a lot of places, so we decided to define a variable for it to make it more obvious to the reader. I'm not sure if I like this pattern or not, but I'm interested to get your thoughts. I think it's all the same to mypy.
https://github.snooguts.net/reddit/reddit-service-funding-instruments/blob/1d490bbd9c79d0329f34884dcc353ab8d413a805/funding_instruments/common_types.py#L4
https://github.snooguts.net/reddit/reddit-service-funding-instruments/blob/1d490bbd9c79d0329f34884dcc353ab8d413a805/funding_instruments/models/sql.py#L7
There was a problem hiding this comment.
Yeah! Then you can just import those type defs too. That should be really nice. I'll get some aliases going.
|
|
||
| _NOT_LOADED = object() | ||
| class _NOT_LOADED: | ||
| pass |
There was a problem hiding this comment.
I went down a rabbit hole looking at this change. It looks like they recommend using an enum for these singleton instances since they can't be subclassed:
https://github.com/python/typing/pull/240/files
I like the way you've done it more tbh, but wanted to point out that the pep recommends something else. I'm totally okay with what you've got, but wanted to share
There was a problem hiding this comment.
Ah yeah, I saw the enum thing. But this comment linked to an implementation ticket in mypy that is still open: python/mypy#1803
so i think it doesn't work yet? didn't actually try because I'm a jerk. Would you prefer I give the proper way a shot?
There was a problem hiding this comment.
Yours feels cleaner and I prefer it. I don't think anyone is going to subclass this _NOT_LOADED class and get confused.
| return super()._get_data() | ||
|
|
||
| def _get_data(self): | ||
| def _get_data(self) -> Dict: |
There was a problem hiding this comment.
I'm guessing we don't know anything about what's in this dictionary?
There was a problem hiding this comment.
yeah, we could get a bit more precise in that the top level will be str->dict but then that underlying dict will be a lot more undefined. not sure how precise we need to be.
pacejackson
left a comment
pacejackson
left a comment
There was a problem hiding this comment.
Just a few small comments
| self.__dict__ = self | ||
|
|
||
| def __getattr__(self, name: str) -> Any: | ||
| ... |
There was a problem hiding this comment.
It's a type stub for __getattr__ so that type checking knows we can return anything we want via attribute references. Prior art is here:
There was a problem hiding this comment.
tl;dr "good luck!!!!!! i'll do whatever i want!!!!"
baseplate/crypto.py
Outdated
|
|
||
|
|
||
| def validate_signature(secret, message, signature): | ||
| def validate_signature(secret: "VersionedSecret", message: str, signature: bytes) -> SignatureInfo: |
There was a problem hiding this comment.
I don't think this should need the quotes right?
|
💇♂️ |
We're Py3 only now. Don't falsely advertise Py2 support.
hmac.compare_digest is available in all versions of Python we support.
3 participants
This brings us from 32% "imprecise" to 28%. Progress.
I'm not super happy with all of this and it definitely shows places where it's really hard to type stuff because it's nice and dynamic. We'll basically never get to have strict types for the request/context object. Bummer.
👓 @pacejackson @bradengroom