davidhewitt / pythonize

MIT License
207 stars 28 forks source link

Support serialising to named mappings #62

Closed juntyr closed 3 months ago

juntyr commented 6 months ago

This PR includes four changes:

I have found them to be very useful to serialise to and from class-like Python types (e.g. namedtuple which keep more type information around).

juntyr commented 6 months ago

@davidhewitt what are your thoughts on this?

juntyr commented 5 months ago

@davidhewitt I apologise for pinging again - I'm hoping that this PR could move forward soon-ish as I'm working on publishing a crate of mine (which would need a feature similar to this) for a publication.

MarshalX commented 5 months ago

@juntyr Perhaps he unfollowed this repository or pull request. Maybe try pinging him on X, for example, as he has a few social links on his GitHub profile

davidhewitt commented 5 months ago

Hi @juntyr sorry for the delay! I have seen this PR but haven't had a chance to investigate yet. Please hold on and I'll do my best to get to this soon - I'm trying to wrap up PyO3 0.22 and then let's aim to ship this in pythonize 0.22 👍

juntyr commented 5 months ago

@davidhewitt Congrats on shipping pyo3 v0.22, I'm excited to start using it!

I'm looking forward to collaborating with you to push this PR forward :)

juntyr commented 4 months ago

The merge conflict should now be resolved again :)

juntyr commented 4 months ago

Once #66 is merged, I will very quickly rebase this PR again so that we can hopefully still get it into v0.22

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 75.25773% with 24 lines in your changes missing coverage. Please review.

Project coverage is 83.41%. Comparing base (6c2c3f2) to head (7c18b9f).

Files Patch % Lines
src/ser.rs 77.77% 4 Missing and 16 partials :warning:
src/de.rs 42.85% 0 Missing and 4 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #62 +/- ## ========================================== - Coverage 83.83% 83.41% -0.42% ========================================== Files 3 3 Lines 1169 1224 +55 Branches 1169 1224 +55 ========================================== + Hits 980 1021 +41 - Misses 140 144 +4 - Partials 49 59 +10 ```

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

juntyr commented 4 months ago

GATs were only stabilised in 1.65 but pythonize uses 1.63 … so the builder can’t use a GAT. Perhaps I can inline the builder methods into the main trait itself (hopefully while keeping Map = PyDict)

juntyr commented 4 months ago

I also tried to add a small convenience wrapper type PythonizeUnnamedMappingWrapper so that

  1. it is more explicit that type NamedMap = PythonizeUnnamedMappingWrapper<PyDict> discards the name
  2. it is reusable for others so you don't need to copy-past two trait impls
juntyr commented 3 months ago

Thanks for your review @davidhewitt!

juntyr commented 3 months ago

I’m unsure if the coverage can be easily improved as all new misses seem to be partial and short-circuiting related

davidhewitt commented 3 months ago

Agreed, I'm very happy to call this as good as it gets! Thanks again 🚀