typemytype
Type annotations
The aim of this pull request is to add type annotations to the drawBot public interface. Here are the main things @typemytype and I worked on:
- Adding annotations and fix the bugs the annotations exposed
- Adding annotations to the tools/imageObject.py module did not seem clever, since it's automatically generated from the Core Image Filters. @typemytype found the original Python script on his drive and we updated it to support annotations and add general improvements.
- On the same line, the
drawBot.__init__.pyfile is partially generated to expose the instance methods outside, allowingfrom drawBot import whateverand benefiting from the annotations in the code editor
Additionally, we made some other minor fixes:
- Removed wildcard imports from
drawBottest scripts (type annotations benefit) - Updated the image URL (from Cloudflare to GitHub) and related test images
- Updated the README
Some tests are failing on my OS (Sonoma 14.4.1). Looking at the difference.pdf, it mostly seems to be related to hinting, hyphenation, text baseline shift, and blending of colors. Depending on the OS GitHub will use to run the tests, the tests might keep failing or not.
We still need to generate automatically basic tests for the ImageObject class. We plan to work on it next week.
Great stuff!
Thanks : )
I am missing a mypy check step in the GH Actions workflow files. How do you currently check the correctness?
I use the Pylance extension in VSCode. I have no experience in setting it into GH Actions. There's a ton of errors we are just ignoring, I think it would be problematic to run it outside a code editor...
I think it would be problematic to run it outside a code editor...
That would make the project depend on VS Code, which is highly undesirable. We need a way to guarantee the type annotations stay correct without VS Code. That really is a hard requirement.
I have never heard of pylance, but we should either use it to do checking in GH Actions (and possibly ignore certain errors, if we know for sure they are not relevant to us), or we should use mypy. I have experience with the latter, is easy-ish to set up, but can be quite picky in terms of type checking.
I just read that pylance is powered by pyright, which is available on PyPI.
If pyright is easier than mypy, I'm totally fine with that, but we're going to have to use either of those as part of automated testing.
I think it would be problematic to run it outside a code editor...
That would make the project depend on VS Code, which is highly undesirable. We need a way to guarantee the type annotations stay correct without VS Code. That really is a hard requirement.
I have never heard of pylance, but we should either use it to do checking in GH Actions (and possibly ignore certain errors, if we know for sure they are not relevant to us), or we should use mypy. I have experience with the latter, is easy-ish to set up, but can be quite picky in terms of type checking.
I just read that pylance is powered by pyright, which is available on PyPI.
If pyright is easier than mypy, I'm totally fine with that, but we're going to have to use either of those as part of automated testing.
I agree on the fact that the codebase should be IDE agnostic. The problem is that we're just annotating a very small part of it. For example, on drawBotDrawingTools.py I now see 72 type errors that I am ignoring. So, if we want to add some automatic checking on GH Actions, it should be narrowed to a portion of the codebase.
Briefly playing with mypy on your branch: it has a problem with Point = Size = tuple[float, float], but if I split that in two:
Size = tuple[float, float]
Point = tuple[float, float]
After that it "only" gives 139 errors, and that seems (perhaps naively) managable.
It did immediately discovered a real bug, though: in imageObject.py, there are many occurences of self._addFilter(filterDict) where the preceding definition for filterDict ends with a comma, turning the dict into a one-element tuple. For example on line 508.
I invoked mypy like this:
mypy --ignore-missing-imports drawBot
it should be narrowed to a portion of the codebase
Mypy is pretty good with ignoring things that aren't annotated.
FWIW, running pyright drawBot --skipunannotated gives:
630 errors, 8 warnings, 741 informations
Files to include in the typechecking:
-
drawBot/__init__.py -
drawBot/aliases.py -
drawBot/drawBotPackage.py -
drawBot/context/baseContext.py -
drawBot/context/tools/imageObject.py -
drawBot/context/tools/drawBotbuiltins.py -
drawBot/drawBotDrawingTools.py
I've modified my setup to work with mypy. I'll start to go through the errors, highlight what needs to be fixed and ignore what's not feasible.
@typemytype and I worked today to fix the typing issues in the public interface. MyPy has been set as a step in the test workflow, so we almost ready! We plan to add some very basic tests for the ImageObject methods next week.
this looks good to me!
sphinx doc building is also adjusted and improved
How is it going with the review process?
Apologies! I will continue soon.
Hey @justvanrossum! @typemytype and I fixed the issues you highlighted. You can proceed with the review 🤓
I would like to finish this PR :)
I can imagine! :)
There's still a bunch of unresolved feedback, though. (And some feedback that has been resolved, but not marked as resolved.)
I think I resolved all the issues. There are commit links too. Am I missing something?
To be honest, looking to outdated hidden message is a bit confusing... but hurray found them deep inside the comment stream!
@justvanrossum I should have answered to all the issues. Please, if there's something missing tag me directly, as the PR interface is getting a bit confusing, thanks!
Hey hey! Let me do if I can do anything to move the ball forward : )
Hey hey, I recently pushed a couple of little improvements. I am using this fork in production in a couple of projects and I'd really love to see it merged upstream. 👋
I know there are probably a lot of different priorities, but is there any perspective for this merge to happen?
Hey Rafal! Meanwhile you can use my fork, I've been using it in production for a while now.
Sorry for my sluggishness. @typemytype: maybe we should just merge, since all tests seem to pass? We can refine later if we run into issues.