wasabi icon indicating copy to clipboard operation
wasabi copied to clipboard

Migrate codebase to support py36 and above

Open ljvmiranda921 opened this issue 3 years ago • 1 comments

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 9 and black to format the codebase
  • Added mypy to requirements.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 **settings approach) 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!

ljvmiranda921 avatar Mar 23 '22 02:03 ljvmiranda921

Yay ready for review! Can't seem to tag reviewers so sorry for the message tag cc: @ines

ljvmiranda921 avatar Jul 26 '22 10:07 ljvmiranda921

Does the CI run mypy in the test suite?

svlandeg avatar Aug 18 '22 10:08 svlandeg

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).

adrianeboyd avatar Aug 19 '22 08:08 adrianeboyd

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.

ljvmiranda921 avatar Aug 19 '22 08:08 ljvmiranda921

You need to merge in master and resolve the conflicts for the CI to run again.

adrianeboyd avatar Aug 19 '22 08:08 adrianeboyd

Ah, the CI hasn't been updated since it got moved to explosion so it's not working. Let me try to fix it...

adrianeboyd avatar Aug 19 '22 08:08 adrianeboyd

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 🙇

ljvmiranda921 avatar Aug 19 '22 08:08 ljvmiranda921

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?

adrianeboyd avatar Aug 19 '22 09:08 adrianeboyd

I noticed that closing and opening the PR re-triggers the CI. Earlier when I made a commit, the CI didn't trigger.

ljvmiranda921 avatar Aug 23 '22 09:08 ljvmiranda921

Noted on the docstrings, will double check them again.

ljvmiranda921 avatar Aug 24 '22 03:08 ljvmiranda921

Converting this to draft again to just double-check all docstrings.

ljvmiranda921 avatar Aug 24 '22 03:08 ljvmiranda921

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! 🙇

ljvmiranda921 avatar Aug 26 '22 00:08 ljvmiranda921

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.

adrianeboyd avatar Aug 26 '22 08:08 adrianeboyd

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

ljvmiranda921 avatar Aug 29 '22 00:08 ljvmiranda921

Looks good!

adrianeboyd avatar Sep 01 '22 11:09 adrianeboyd