Skip to content
This repository was archived by the owner on Apr 26, 2019. It is now read-only.

encoding/dot: new package DOT graph encoding#57

Merged
kortschak merged 1 commit intomasterfrom
dot
May 25, 2015
Merged

encoding/dot: new package DOT graph encoding#57
kortschak merged 1 commit intomasterfrom
dot

Conversation

@kortschak
Copy link
Member

Currently, the package handles our basic graph types without any problem, but there are features in DOT that we can use (the additional interfaces do this - though they are not yet tested), e.g. subgraphs.

When this has been iterated over, I will write a DOT parser to do the unmarshaling (this has some hairy bits that I'm still thinking about, but it's less of a concern - I just want something to make our packages a bit more friendly for visualisation).

@gonum/developers

@kortschak kortschak force-pushed the dot branch 6 times, most recently from 0a4cd39 to c7e2b40 Compare May 21, 2015 03:03
@kortschak kortschak force-pushed the dot branch 2 times, most recently from 9b575cc to fd82995 Compare May 21, 2015 04:52
@kortschak
Copy link
Member Author

This is ready for some review.

Current issues are:

  1. We don't check for valid DOT ID syntax.
  2. We don't really properly handle subgraphs - the addition of the strict option is to deal with that (we just pass the problem on to GraphViz). I don't think this is a huge issue since subgraphs are unlikely to be a common use case, and we can (I think at this stage) legitimately just force all graphs to not be pseudo-graphs (note that I'm still not convinced that pseudo graph support is not a worthwhile goal in the long run) by adding strict. Maybe fixed.
  3. The code could be a lot cleaner.

PTAL

@kortschak
Copy link
Member Author

ping

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

global?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. That should not be there.

For reference, all this comes from this and playing around.

@vladimir-ch
Copy link
Member

I think it LGTM.

@kortschak
Copy link
Member Author

PTAL for the documentation wording.

I guess we can clean up the code at a later stage. It's reasonably readable, but I do think it could be improved upon.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that this is stated explicitly.

@vladimir-ch
Copy link
Member

Much better now. LGTM.

@kortschak kortschak merged commit ac513d9 into master May 25, 2015
@kortschak kortschak deleted the dot branch May 25, 2015 06:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants