search

davidhewitt / pythonize

MIT License
207 stars 28 forks source link

Update to PyO3 0.22 #66

Closed HenningHolmDE closed 4 months ago

HenningHolmDE commented 5 months ago

This bumps the PyO3 dependency to the new 0.22 release.

As PyNativeType (used in src/de.rs) has now been moved behind the gil-refs feature, I added that feature to the list of PyO3 feature dependencies. As Clone (used in tests/test_custom_types.rs) has now been moved behind the py-clone feature, I added that feature to the list of PyO3 feature dependencies for development.

However, if you would rather go down a different route with the upstream changes than just adding the required features (e.g. removing the corresponding deprecated functions instead), I'm happy to change the PR accordingly.

HenningHolmDE commented 5 months ago

As it turns out, simply enabling gil-refs seems not the right way to go, as it introduces trait bounds to HasPyGilRef for downstream projects as well.

I will see, if I can come up with a better way.

Switched to draft until this is resolved.

HenningHolmDE commented 5 months ago

Second try. I now reverted using the gil-refs feature and removed all code that relied on PyO3 functionality that has been moved behind the gil-refs feature gate in the new release.

I'm sorry for the seesaw and hope that the change makes more sense now.

juntyr commented 5 months ago

I personally think that as long as Pyo3 still has the pre-bound methods, pythonize should probably have them as well.

What about adding a gil-refs feature to pythonize that gates the old methods and enables that feature inside Pyo3?

On the other hand I can also understand waning to reduce the maintenance burden earlier in this smaller helper crate.

HenningHolmDE commented 5 months ago

I personally think that as long as Pyo3 still has the pre-bound methods, pythonize should probably have them as well.

What about adding a gil-refs feature to pythonize that gates the old methods and enables that feature inside Pyo3?

I would agree, adding a gil-refs feature seems like the better solution over removing the code as long as the required functions are still available in PyO3.

On the other hand I can also understand waning to reduce the maintenance burden earlier in this smaller helper crate.

Also, this would extend the CI effort a bit, as we would have to ensure building all feature "combinations" does work.

Anyone else thoughts on this?

deedy5 commented 4 months ago

up

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 94.04762% with 5 lines in your changes missing coverage. Please review.

Project coverage is 83.83%. Comparing base (30ce873) to head (e7cf258).

Files Patch % Lines
src/ser.rs 93.97% 5 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #66 +/- ## ========================================== + Coverage 81.25% 83.83% +2.58% ========================================== Files 3 3 Lines 1152 1169 +17 Branches 1152 1169 +17 ========================================== + Hits 936 980 +44 + Misses 167 140 -27 Partials 49 49 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

davidhewitt commented 4 months ago

Sorry for the long delay here. Thanks for the PR and getting this moving! I think it's ok to remove the gil-refs methods as we'll be removing them in the next PyO3 release anyway.

I'll get this merged and released ASAP. Have pushed some commits to finish off & fix conflicts.

HenningHolmDE commented 4 months ago

Thanks for your reply and no worries about the delay. I guess, most of us are doing this in our spare time. ❤️

If you run into anything that needs further work, just let me now, I'm happy to support.