Open roberto-arista opened 6 months ago
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.
It took a while, but finally all tests are passing : )
🙌 super!! thanks
thank you for all the support and guidance
this looks good to me!
sphinx doc building is also adjusted and improved
How is it going with the review process? @typemytype @justvanrossum
How is it going with the review process?
Apologies! I will continue soon.
I'll go through them on Monday!
Hey @justvanrossum! @typemytype and I fixed the issues you highlighted. You can proceed with the review 🤓
@justvanrossum I would like to finish this PR :)
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?
Just go to “show hidden” and within that one more time.
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! Can I do anything to move the ball forward?
Hey hey! Let me do if I can do anything to move the ball forward : )
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:drawBot.__init__.py
file is partially generated to expose the instance methods outside, allowingfrom drawBot import whatever
and benefiting from the annotations in the code editorAdditionally, we made some other minor fixes:
drawBot
test scripts (type annotations benefit)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.