Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.1k
Convert ft2font extension to pybind11#28785
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
QuLogic commentedSep 7, 2024
I do not understand the remaining stubtest failures; I'm also not sure what to do about |
ksunden commentedSep 7, 2024
Okay, so on current main, The methods which return a Glyph do work, and return something that the repr says it should be at This PR changes that such that the Python classdoes exist. As for the metaclass, my guess is that that is effectively the same problem. My leaning would be go ahead and add a constructor, it doesn't hurt anything and may solve these issues. That or see if you can omit Glyph from the PYMODULE and it just works. I'm not sure why stubtest was not erroring about the class not existing at runtime previously, but it wasn't. The type hints see no |
408b128 to2079fc8CompareQuLogic commentedSep 10, 2024
For For the metaclass, I added |
Inline `convert_xys_to_array` and modify the arguments to take a C++container, so we don't need a less-safe pointer, and we don't need tocopy another time over.
This allows pybind11 to generate the type hints for us, and it alsotakes care of checking the list and its contents are the right type.
The former is not available on the macOS deployment target we use forwheels. We could revert back to `PyOS_snprintf`, but C++11 contains`snprintf`, and it seems to guarantee the same things.
And also ignore the `numpy.float64` reference. The latter seems to bebroken since Sphinx tries to auto-link type hints as `py:class`, butit's an alias in NumPy making it a `py:attr` in their inventory.
QuLogic commentedSep 12, 2024
Reverted that, since it causes weird results in the docs. Now |
| defset_size(self,ptsize:float,dpi:float)->None: ... | ||
| defset_text( | ||
| self,string:str,angle:float= ...,flags:int= ... | ||
| )->NDArray[np.float64]: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm wondering ifnp.double is the correct return type?
https://numpy.org/doc/stable/reference/arrays.scalars.html#numpy.double
From reading the documentation it seems like float64 is an alias for double onsome (probably most though) platforms.
(Not added in the PR, but the missing reference is...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good thought, but unfortunately it doesn't fix the missing reference. It appears that Sphinx doesn't read any.pyi files (which is why most other pages don't have type hint issues), but in this case is seeing something thatpybind11 is generating in the runtime itself.
ianthomas23 commentedSep 18, 2024
I was going to say that we shouldn't need |
| } | ||
| PyFT2Font *self =newPyFT2Font(); | ||
| self->x =NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ideally we'd use the C++nullptr instead ofNULL, but as there are a number ofNULLs already in this file let's leave it consistent with that.
ianthomas23 commentedSep 18, 2024
Merging as there are 2 approvals. I intend to make time to review#27011 sometime in the next week. |
tacaswell commentedSep 18, 2024
My understanding is that@QuLogic has some follow on work to finish removing our build-time numpy dependency all together. |
PR summary
Unlike#9763, this is not a full rewrite, but more of a refactor. There may be further cleanup possible in the future, but this is somewhat large enough already.
PR checklist