Issue/363 type hints #435
Issue/363 type hints #435
Conversation
Added new information on type hinting in the contribution guidelines. Includes examples for contributors who have never added type hints to maintain consistency. Includes a section on remaining Python 3.6 compatible with type hints.
The `<Canvas>` module has had Python 3.6 compliant hints added. This will allow IDEs to display type hints to users as they work. The contribution guidelines have been updated to show the preferred method of adding type annotations gradually.
|
So, this failed because of the |
Thetwam
added
the
hacktoberfest-accepted
Thetwam
removed
the
hacktoberfest-accepted
There was a problem hiding this comment.
Hey @bennettscience, thank you for this patch. I'm sorry for the radio silence on it.
I've had an opportunity to experiment with mypy in some other projects and I think I finally have some useful feedback. Please take a look at the comments and let me know what you think when you get a chance.
| return AppointmentGroup(self.__requester, response.json()) | ||
| ``` | ||
|
|
||
| To get around this problem, the `typing` package has a `TYPE_CHECKING` module |
There was a problem hiding this comment.
Is it possible to use forward references instead of conditional importing? I notice you use them later, but I'm not sure if the conditional imports are a requirement for forward references to work or not.
There was a problem hiding this comment.
I'd tried that, but if I remember correctly, I had inconsistent results...mypy still threw errors. But, I could be remembering that wrong. I'll pull the last update back into this branch and see what happens.
| @@ -230,7 +250,9 @@ def create_appointment_group(self, appointment_group, **kwargs): | |||
|
|
|||
| return AppointmentGroup(self.__requester, response.json()) | |||
|
|
|||
| def create_calendar_event(self, calendar_event, **kwargs): | |||
| def create_calendar_event( | |||
| self, calendar_event: dict, **kwargs: Union[str, dict, Optional[dict]] | |||
There was a problem hiding this comment.
Matt and I have had a few conversations about this and we're not sure what the best approach is: how do we handle kwargs?
One of the nice things about taking in arbitrary keyword arguments is that canvasapi can maintain parity with Canvas updates as long as the endpoints themselves don't change. The parameters might change types or names or expect new values, but there's nothing we have to do downsteam.
Of course, the downside to that is that there's no useful information for developers in the function signature about what Canvas might accept. Typehints partially address this, but they reintroduce the API parity problem.
Unless we can come up with a good tradeoff, I wonder if our kwargs typehint needs to be Any or something equally permissive.
There was a problem hiding this comment.
Yeah, I did a lot of reading on how to type **kwargs and there isn't really a broad consensus. For me, I'm typically passing those in as a dict, but Any also makes sense. It's not considered bad practice because it really could be anything. Maybe a note in the docs somewhere about the Any type being limited to kwargs specifically?
| @@ -498,7 +545,7 @@ def get_appointment_group(self, appointment_group, **kwargs): | |||
| ) | |||
| return AppointmentGroup(self.__requester, response.json()) | |||
|
|
|||
| def get_appointment_groups(self, **kwargs): | |||
| def get_appointment_groups(self, **kwargs: Optional[dict]) -> PaginatedList: | |||
There was a problem hiding this comment.
Is it possible to annotate with PaginatedList[AppointmentGroup]
There was a problem hiding this comment.
Yes, you can do that directly or through a Type Alias to simplify complex returns.
Proposed Changes
Include type hints for the
<Canvas>class.This is a first draft of including type annotations in the library. The contribution guidelines have also been updated to show how to add annotations to other modules following Python 3.6 formatting for backwards compatibility. I've tagged the original issue for reference, but this PR only includes annotations for
<Canvas>at this point.Related issue: #363