The project now typechecks, improved IntelliSense #164
Conversation
ConnorMcMahon
left a comment
ConnorMcMahon
left a comment
There was a problem hiding this comment.
Looks mostly good, but I have some questions, likely due to me not knowing how type annotations in Python work.
| instance_id=instance_id, orchestration_function_name=orchestration_function_name) | ||
|
|
||
| response = await self._post_async_request(request_url, self._get_json_input(client_input)) | ||
| response: List[SerializableToJSON] = await self._post_async_request( |
There was a problem hiding this comment.
What was the conclusion about SerializableToJSON. I'm not sure about how Python feels about custom types like this to represent attributes of a type?
There was a problem hiding this comment.
I recently learned that the right approach here is to use typing Protocols [https://mypy.readthedocs.io/en/stable/protocols.html] which allows us to typecheck if some type exports some method. If so, we can implement SerializableToJSON as the union of some primitive types plus objects that export to_json + from_json methods.
However, that's a longer term goal. For now, I've changed SerializableToJSON back to Any, with the goal of merging this and revisiting the type in the future :)
ConnorMcMahon
left a comment
ConnorMcMahon
left a comment
There was a problem hiding this comment.
I think it looks good, just a couple last things to consider.
Well typed programs don't go wrong ....
Do not merge yet, I want to double check with some of our internal Python experts if our use of the
Optionaltype hint, and type aliasing is idiomatic. However, feel free to reviewThis PR constitutes, to the best of my knowledge, the first type-checking of our codebase. As per the
mypytype-checker, our codebase is type-safe and. Most of the work required to get there revolved around acknowledging and guarding against theNonetypes that flow through our code. In fact, after this effort, I'm really concerned about the amount ofNoneinitialization that we do in our codebase. As a result, I'd love to speak with some of our internal Python experts about providing sane initialization values to non-object types. Especially because initializing all optional values asNoneis, on the flipside, very convenient for guarding against them.Something else I attempt in this PR, although very minimally, is the idea of introducing
type aliasesto provide more information around primitive types. For instance, many functions returns URL-formatted strings and thus the return type ofstris not very informative, in my opinion. In languages like Haskell, it's common to alias these types to better document functions, therefore resulting in an aliasing ofstrto a new name likeURLstr. I have yet to figure out how pythonic that really is. In this PR, I uses a type alias forAnycalledSerializableToJSONin hopes of conveying that some inputs can be of any type so long as they're serializable. Again, I need to check with an expert if this is idiomatic. If so, I'll propagate theSerializableToJSONdatatype all over the codebase