Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type annotations #570

Open
wants to merge 88 commits into
base: master
Choose a base branch
from
Open

Conversation

roberto-arista
Copy link
Contributor

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__.py file is partially generated to expose the instance methods outside, allowing from drawBot import whatever and benefiting from the annotations in the code editor

Additionally, we made some other minor fixes:

  • Removed wildcard imports from drawBot test 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.

@roberto-arista roberto-arista changed the title [WIP] Type annotations Type annotations Jun 21, 2024
@typemytype
Copy link
Owner

🙌 super!! thanks

@roberto-arista
Copy link
Contributor Author

thank you for all the support and guidance

@typemytype
Copy link
Owner

this looks good to me!

sphinx doc building is also adjusted and improved

@roberto-arista
Copy link
Contributor Author

How is it going with the review process? @typemytype @justvanrossum

@justvanrossum
Copy link
Collaborator

How is it going with the review process?

Apologies! I will continue soon.

Copy link
Collaborator

@justvanrossum justvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started to do a new review, and left some comments. But then I came across my earlier feedback which seems to have not been addressed at all, so I'll stop here.

drawBot/context/svgContext.py Outdated Show resolved Hide resolved
drawBot/__init__.py Show resolved Hide resolved
drawBot/drawBotDrawingTools.py Outdated Show resolved Hide resolved
drawBot/context/__init__.py Outdated Show resolved Hide resolved
drawBot/context/baseContext.py Outdated Show resolved Hide resolved
@roberto-arista
Copy link
Contributor Author

I'll go through them on Monday!

@roberto-arista
Copy link
Contributor Author

Hey @justvanrossum! @typemytype and I fixed the issues you highlighted. You can proceed with the review 🤓

@typemytype
Copy link
Owner

@justvanrossum I would like to finish this PR :)

@justvanrossum
Copy link
Collaborator

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

@roberto-arista
Copy link
Contributor Author

I think I resolved all the issues. There are commit links too. Am I missing something?

@justvanrossum
Copy link
Collaborator

Just go to “show hidden” and within that one more time.

@typemytype
Copy link
Owner

To be honest, looking to outdated hidden message is a bit confusing... but hurray found them deep inside the comment stream!

@roberto-arista
Copy link
Contributor Author

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

@roberto-arista
Copy link
Contributor Author

Hey! Can I do anything to move the ball forward?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants