Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
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
I do not understand the remaining stubtest failures; I'm also not sure what to do about |
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 to2079fc8CompareFor 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.
Reverted that, since it causes weird results in the docs. Now |
| @@ -232,23 +215,73 @@ class FT2Font: | |||
| def set_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.
I was going to say that we shouldn't need |
| } | ||
| PyFT2Font *self = new PyFT2Font(); | ||
| 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.
Merging as there are 2 approvals. I intend to make time to review#27011 sometime in the next week. |
9674b53 intomatplotlib:mainUh oh!
There was an error while loading.Please reload this page.
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