Make dictionary class and list class optable

[i1i1]

Main motivation for dictionary and list types is to be able to send this kind of datastructures:

#[derive(PartialEq, Eq, PartialOrd, Ord)
struct Id {
   a: String,
   b: String,
}

BTreeMap<Id, i32>

So here BTreeMap has key type as Id which will be mapped to python dictionary which is not hashable. So in order to be able to deserialize more things from python it would be nice to replace PyDict with this dict.

[davidhewitt]

Thank you for this PR. I can understand the use-case you are aiming to solve by introducing this new type.

However I am not convinced I support it for this library. For example, if you are using serde_json, you are only able to use strings as map keys. Similarly here, the expectation so far is that we are only able to use types which Python considers hashable.

By introducing this new type, you are able to remove this constraint. However, there are new problems, for example it's no longer possible in Python to json-encode the output of this library by default. Many of the tests in this library are failing as a result.

I would be more okay with this change if there was a way to opt-in to it rather than make this by default. I think most users would prefer to have PyDict rather than a custom type.

As this is, I'm afraid I'm unenthusiastic about merging, however I'm always open to being persuaded otherwise...

[i1i1]

@davidhewitt Thanks for the feedback!

Considering serde_json, its goal is to comply json format and it doesn't support keys to be json objects themselves. While serde itself has data model and it does not specify that.

Wasn't thinking about the defaults during making it. Would you consider adding it if i make it a cargo-feature?

[davidhewitt]

A cargo feature is a great start so that users who don't need it don't pay for it. However, this also needs to be a different API e.g. pythonize_with_custom_dict, because otherwise someone who enables this cargo feature would silently change all other users of pythonize (because of the way cargo features are unified).

Tbh I would be more comfortable with this addition if it was built so that users could swap the target Dict and List containers as they liked. So that instead of having to choose between PyDict or this new Dict type, they could also supply their own custom mapping. Some kind of PythonizeOptions or PythonizerBuilder might be able to do this.

[i1i1]

I see. What do you think about implementing it as generic then?

We could expose some trait (which we implement for PyDict and this dictionary from pr). Pythonize and depythonize would work using PyDict, while some other api (like pythonize_custom or some other name) would let users to specify their own dictionary (via generic).

[davidhewitt]

A generic sounds a lot like the kind of thing I was envisioning.

It should be easier to write depythonize (as python input can be duck-typed). A full trait description would probably be necessary for pythonize.

[i1i1]

@davidhewitt Sorry for delay. Updated pr with traits for List and Dict. Now you can supply custom dict and list with (de)?serialize_custom api. Could you rereview pr and share your thoughts?

[davidhewitt]

🚀 Thanks for revisiting this! The new iteration looks great, overall I'm really happy with the flexibility this gives.

I'm a bit tired tonight so haven't managed to give as much detail to the review as I would have liked, however I have a few initial suggestions. I think that they should simplify the code a little, and then I'll aim to give this once final review (with the intention to merge).

[davidhewitt]

Thanks very much, this is shaping up well.

I've given this a detailed read through and overall am happy with how this expands the functionality of pythonize.

I think the traits are more complicated than they need to be, so I think there might be a couple of rounds of iteration still to go here to simplify what pythonize asks users to implement. Once the traits are good, I'm all for merging and releasing this!

In particular I think the PyArray trait (which I think could be called PythonizeSequence) already can be simplified a lot, and if we build a PyMapping helper (see comment below) then we can do a similar thing for PyDictionary.

[codecov-commenter]

Codecov Report

❗ No coverage uploaded for pull request base (main@48c8a8a). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #16   +/-   ##
=======================================
  Coverage        ?   82.40%           
=======================================
  Files           ?        4           
  Lines           ?      938           
  Branches        ?        0           
=======================================
  Hits            ?      773           
  Misses          ?      165           
  Partials        ?        0           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 48c8a8a...94b531d. Read the comment docs.

[i1i1]

@davidhewitt sorry for delay. Here is a new version of pythonize which uses both mapping and sequence protocol as we discussed (Resolved previous issues also).

There are 2 commits here:

• one updates version of pyo3
• another one with actual changes

[davidhewitt]

Thanks for the many many rounds of iteration this PR has gone through, I'm satisfied that this new implementation should provide a nice way for users to supply custom classes without adding too much baggage to the pythonize core.

Really great work 🎉

I'll release this once PyO3 0.15 is out and we can update pythonize to depend on it fully.