wasabi
wasabi copied to clipboard
Migrate codebase to support py36 and above
Description
This PR attempts to update the wasabi codebase so that it's compatible to Python 3.6 and above. Major changes include:
- Added type hints to all modules
- Removed Python27 from the CI
- Ran
isort -m 9andblackto format the codebase - Added
mypytorequirements.txt, and included it in the CI.
A note on typing and mypy
There are areas in the codebase where I prioritized readability than mypy's checks, By doing so, there are some lines of code where I added type: ignore or casted some variables to the appropriate types. Let me know if there's a better way to approach these. Here are some examples:
- Ignored type when using
**kwargs: https://github.com/ines/wasabi/pull/22/commits/f8f3c595afad55e2fa39366f88541ae88d21f8d1. Here, I prioritized neatness (I prefer the original**settingsapproach) rather than passing the variables one by one. - Fix dictionary getters by casting: https://github.com/ines/wasabi/pull/22/commits/534311653f2f6b0b73b1a6d363cb5f9190d0d8ed. Here, I opted to cast the variables instead of writing overloads.
Also, note that I'm not as familiar with typing and mypy best practices, so let me know if there are things I missed! Would appreciate some pointers!
Yay ready for review! Can't seem to tag reviewers so sorry for the message tag cc: @ines
Does the CI run mypy in the test suite?
Please fix CI steps first and address the mypy errors (which are currently printed in the CI but don't cause the step to fail).
Oh my mistake @adrianeboyd , I re-requested the review but forgot to push the changes! I have pushed them now and will wait for the CI to finish. Once everything passes (or once I fixed any errors), I will request the review again. :) I apologize for that.
You need to merge in master and resolve the conflicts for the CI to run again.
Ah, the CI hasn't been updated since it got moved to explosion so it's not working. Let me try to fix it...
Ok! I reran mypy locally and encountered two errors:
λ: python3 -m mypy wasabi
wasabi/util.py:210: error: Module has no attribute "WinDLL"
wasabi/util.py:230: error: Module has no attribute "get_osfhandle"
Found 2 errors in 1 file (checked 13 source files)
I will solve these first in the meantime 🙇
That code should have been removed in the merge? It doesn't matter to me if this PR gets rebased, so it might be easier to revert back to before the merge and rebase on master instead?
I noticed that closing and opening the PR re-triggers the CI. Earlier when I made a commit, the CI didn't trigger.
Noted on the docstrings, will double check them again.
Converting this to draft again to just double-check all docstrings.
Hi @adrianeboyd ! I'm not sure if I have pending changes for the PR. I tried re-opening some resolved threads, perhaps a comment was buried or deleted somewhere? Let me know if there are things I need to update or if I should click the "request re-review" button! 🙇
Ah, with Optional[] I do like the Union version better. Maybe Union everywhere would just be simpler/clearer?
An additional change would be to add python_version=">=3.6" to the setup. We should also redo the setup to use setup.cfg, but I can do that as a follow-up PR.
Ah, with Optional[] I do like the Union version better. Maybe Union everywhere would just be simpler/clearer?
Ah sure. I think Union everywhere is clearer. Will revert this commit: https://github.com/explosion/wasabi/pull/22/commits/fb2c728aa5d095f7b0d4a89cceb55adaf3926fc7
An additional change would be to add python_version=">=3.6" to the setup.
Sure I can add that
We should also redo the setup to use setup.cfg, but I can do that as a follow-up PR.
I made something here to get things started: https://github.com/ljvmiranda921/wasabi/pull/1
Looks good!