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?

[codecov-commenter]

Codecov Report

:exclamation: 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