mwclient
Add Type Annotations to the Codebase
Since dropping Python 2 support in #310, we now have the ability to add type annotations to the codebase. I think this could be very helpful for users to discover the API and to prevent bugs.
This PR is intended as a starting point for discussions (@mwclient/maintainers) and adds basic type annotations to all functions. Since we often return responses from the MediaWiki API verbatim, I've chosen to leave the return type as Any or Dict[str, Any] in many cases. This could be improved, for example, with TypedDicts, but I'm not sure we want to take on that maintenance burden.
Here are some open questions I have regarding this:
- Do we want to maintain types in the first place?
- Which type-checker should we use? I've chosen mypy for now, but we could, for example, use pyright instead.
- Do we want to type responses, for example, using
TypedDict?
coverage: 70.38% (+0.3%) from 70.065% when pulling db4d82ffa12e48d114e1ad6daffb672bbc7c0d1c on feature/add-types into efa0ca5bc5fdd0e8d3b2b1349740b741de1db9ea on master.
- Yes, I think this is a great change!
- I'm not opinionated about this, mypy is fine
- Sadly no I don't think we should type API responses, simply because we support too many MediaWiki versions at once for this to be practical. I'd rather not have typed dicts being returned at all than have some users seeing incorrect typed dicts, and I think it would be too difficult to maintain types for all currently-supported versions of MediaWiki - and even then, I'm sure we have a large number of users on small, private, out-of-date MW versions
I agree with @RheingoldRiver on all points. Thanks for doing the work @marcfrederick !
Great! In that case, I'll remove the draft status from this PR and leave it ready for review. If you have existing projects using mwclient, you should be able to test the annotations by installing from this branch:
pip install git+https://github.com/mwclient/mwclient.git@feature/add-types
coverage: 70.38% (+0.3%) from 70.065% when pulling 067fa95c44558ee9239a350d5cd2722bdb820de9 on feature/add-types into efa0ca5bc5fdd0e8d3b2b1349740b741de1db9ea on master.
coverage: 73.68% (+0.1%) from 73.535% when pulling c77389014172638b90b9e60864d434ff19c2c46f on feature/add-types into c3ed7ae805d7257ad242e3d528a88329bb8fd1d7 on master.
coverage: 70.246% (+0.4%) from 69.874% when pulling a323476d1e41f2179f4edcc1cb99737789e2401b on feature/add-types into da48d446753955a66859a207d239c9f7993b2fef on master.
whew, okay, made it through client.py :P Gonna take a break now.
It's fun how doing typing tends to make you think "what we really need to do is burn this library to the ground and start over", huh.
a general note - I noticed you quote class names throughout the PR. I can't find an authoritative reference on whether this is the best choice or not. it seems like it's only required if the class being referenced might not be defined at the time the type is parsed (e.g. if something in a class can return an instance of that class). do you have any notes on this?
ping @marcfrederick , will you be able to get to rebasing this soon? it'd be awesome to get it merged before we do anything else substantial. thanks!
a general note - I noticed you quote class names throughout the PR. I can't find an authoritative reference on whether this is the best choice or not. it seems like it's only required if the class being referenced might not be defined at the time the type is parsed (e.g. if something in a class can return an instance of that class). do you have any notes on this?
Yes, that's one of the reasons for using quotes. From a typing and mypy perspective, most of the quoted references could be removed. However, this leads to runtime issues, such as:
AttributeError: partially initialized module 'mwclient' has no attribute 'listing' (most likely due to a circular import)
I think fixing this would require a whole lot of restructuring, e.g. at the moment we have a circular import between listing and page. This does not seem to matter much in practice, until I unquote the types. When quoting the class-names, evaluation is delayed and everything works fine. There might be a better way to do this, but I haven't come up with anything as of now.
ping @marcfrederick , will you be able to get to rebasing this soon? it'd be awesome to get it merged before we do anything else substantial. thanks!
Sorry, I was on vacation for the past two weeks. I've rebased this on top of upstream/master, and made some small fixups. I think with that this is more or less ready.
Thanks again! I merged #362 , so this needs one more rebase, then I think we can land it! Thanks for all the work.
Yep, will do. Should we squash this PR before merging/use "Squash and merge"?
yeah, I think a squash is appropriate. I'm fine with doing a squash-and-merge , but if you want to do a manual one for any reason, go ahead. I'll give it a few hours.