Add "serde-1" support to GraphMap #341
Conversation
|
Don't you think we need to look at the whole picture and make this serialize in a format identical to or as similar as possible to, what Graph and StableGraph use? |
| @@ -75,6 +143,7 @@ impl<N> NodeTrait for N where N: Copy + Ord + Hash {} | |||
|
|
|||
| // non-repr(usize) version of Direction | |||
| #[derive(Copy, Clone, Debug, PartialEq)] | |||
| #[cfg_attr(feature = "serde-1", derive(Serialize, Deserialize))] | |||
| enum CompactDirection { | |||
There was a problem hiding this comment.
This enum is internal so it would be good to not expose it in serialization
| deserialize = "N: serde::Deserialize<'de>, E: serde::Deserialize<'de>" | ||
| )) | ||
| )] | ||
| pub struct GraphMap<N, E, Ty> where N: Eq + Hash { |
There was a problem hiding this comment.
The implementation details - which fields are used, of graph map are internal, so it would be good to not expose them. Otherwise make clear in documentation, that serialization of GraphMap in particular, is not stable across semver incompatible versions of the library. Thanks. See other comments, a more deliberate serialization format - not derived - could be appropriate.
You are right - I didn't care much about that up until now. Is there some documation or issue that discusses the format of those? Seems like yet another reason to have a graph trait across all types of containers. OT: I do not like |
|
@schulzch do you have any updates regarding the comments? |
|
The project I'm using petgraph for is currently on hold (but not dead), and I haven't had time to work on a generalizable solution, yet. |
|
Maybe serialize should be implemented on |
This PR propagates
serde-1toindexmapand implements a custom serializer for(N, N)tuples, i.e., edges ([Source, Target, Weight]array format). The custom serializer is required forserde_jsonto work (tuples are not allowed as object keys). Also tested usingbincode.