Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add json serialization for tools. #34

Merged
merged 10 commits into from
Jul 15, 2014
Merged

Add json serialization for tools. #34

merged 10 commits into from
Jul 15, 2014

Conversation

schmmd
Copy link
Member

@schmmd schmmd commented Jul 11, 2014

Resolves #34.

@schmmd schmmd closed this Jul 11, 2014
@schmmd schmmd deleted the json branch July 11, 2014 15:05
@schmmd schmmd reopened this Jul 11, 2014
@dirkgr
Copy link
Member

dirkgr commented Jul 14, 2014

Are you going to add tests for this?

@dirkgr
Copy link
Member

dirkgr commented Jul 14, 2014

Nevermind. Found it.

@dirkgr
Copy link
Member

dirkgr commented Jul 14, 2014

The serialization for the graph is just the edges, with the nodes repeated every time they are part of an edge. I'm not terribly happy about that, for two reasons.

  1. If the nodes get more information on them, we'll waste space having them multiple times. Also, when we deserialize, we'll have to deal with the possibility that we'll have two nodes with the same id, with different content. I could see this happen especially when we manually mess with the serialization in a text editor.
  2. We can't serialize a graph that has a single, disconnected node in it.

@dirkgr dirkgr assigned schmmd and unassigned dirkgr Jul 14, 2014
@schmmd
Copy link
Member Author

schmmd commented Jul 14, 2014

What would you propose? Maybe I should just remove the dependency graph serialization for now.

@dirkgr
Copy link
Member

dirkgr commented Jul 14, 2014

I propose that the serialization of the graph is a list of nodes, followed by a list of edges. The list of edges references nodes by id, or by index into the list of nodes.

That’s how GraphML, GraphSON, and those other formats all do it.

On 2014-07-14, at 13:41, Michael Schmitz notifications@github.com wrote:

What would you propose? Maybe I should just remove the dependency graph serialization for now.


Reply to this email directly or view it on GitHub.

@schmmd schmmd closed this Jul 14, 2014
@schmmd schmmd reopened this Jul 14, 2014
@schmmd
Copy link
Member Author

schmmd commented Jul 14, 2014

I removed the graph serialization so we can use something more standard when we potentially change the underlying graph library.

dirkgr added a commit that referenced this pull request Jul 15, 2014
Add json serialization for tools.
@dirkgr dirkgr merged commit 0b80019 into master Jul 15, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants