Importing .dot files

[amackworth]

:sparkles:

[amackworth]

@robrix I'm actually running into a bit of trouble here, because GraphViz graphs only the structure of the graph, and not its contents. As a result, right now I'm just returning an empty Graph<String>, and I'm not sure what more we can do. :cold_sweat:

Anyways, I'd love to hear your thoughts!

Now that I think about it, the GraphViz files do not and should not hold the semantics about the graph. (After all, it is just a visual representation format.) Therefore, loading in a .dot file should return a Graph<Node> that are all essentially "empty," and the user has the responsibility of assigning them meaning. I'll fix this up real quick, and hopefully have something more useful to show soon!

[robrix]

I’d been thinking this as well. I came to the conclusion that you could import uninterpreted graphs, i.e. something like Graph<(String -> T) -> T> rather than Graph<Node>, such that the graph would be fully imported and structured, and then you’d (essentially) map the node’s label (if any) onto the intended type.

I think this is equivalent to Graph<String> -> Graph<T>, so I think that would probably be fine.

Come to think of it, perhaps we’d instead want to map the dictionary of attributes to T? I.e. Graph<[String: String]>?

[robrix]

A note on style: I am using GitHub’s Swift Style Guide with at least one additional rule about whitespace:

Two blank lines above & one blank line below // MARK: lines, but omit the blank lines above when the // MARK: is at the top of another lexical section, e.g. a struct definition.

[robrix]

This looks fantastic, and I am super grateful for it! :heart_eyes:

Some meta-commentary on my review notes:

• Hopefully my review notes aren’t too premature given that this is a WIP; if you’d rather not have ongoing review until you’re ready, that’s totally fine—feel free to just tell me when you’re ready for review.
• Anything I’m saying “don’t sweat it” on is completely aspirational—it’s a way for me to orient my understanding of these changes with respect to my future goals for the project. Therefore, such comments really needn’t motivate further change.
• Discussion on review notes is always welcome. In particular, don’t be afraid to push back if you think I’m wrong.
• Finally, it’s totally cool by me to e.g. do things ad hoc at present and generalize in a later PR :+1: Scope creep in PRs can be really demoralizing & demotivating, and given that you’re already being super generous with your time, we are both invested in your happiness in this :smile:

(I’ve really got to make a CONTRIBUTING.md file :smile:)

Thank you so much for this excellent work!

[amackworth]

I'm glad you like it! :blush:

Also, thanks for these notes. This kind of commentary is very educational, especially since I'm so new to this project. :heart:

Anyways, I was wondering if you could clarify what you meant by Graph<String> -> Graph<Node> in the comment above. Does this mean that importDOT returns a function that takes a Graph<String> and maps those names across an empty Graph<Node>? Shouldn't it return a function that takes a type and returns a Graph of that type?

[robrix]

You’re welcome!

I meant that importDOT could return Graph<String> (with e.g. the label) or Graph<[String: String]> (with all the attributes), and then the caller could map it to Graph<T>.

[amackworth]

Ah, ok, that makes sense. I'll whip that up real quick!

[amackworth]

@robrix Although still pretty rough, this is now ready for review! :tada:

[robrix]

When you push commits while I’m reviewing, it changes the diffs, making it difficult for me to proceed.

If you wouldn’t mind, it’d be helpful to hold off pushing until I signal that I’m done reviewing—thank you!

[amackworth]

Oh, yes, sorry! :bow:

[amackworth]

Yeah, for some reason it just kept reverting it. Had to go in manually. :unamused:

[amackworth]

And it seems to work with a clean build/test. :relieved:

[robrix]

I gotta pause my review for the afternoon, unfortunately. Feel free to push/tweak.

Quick note: Please use tabs for all source files and try to match code style; I’m fairly intentional about spacing, using trailing closure syntax, etc.

[amackworth]

OK, sounds good. Hope you have a great day! :smile_cat:

[amackworth]

I had to change const(Identifier()) back to { _ in Identifier() } because the former would evaluate immediately, and then populate the array with the same repeated value. I tried to do something with Memo, but that ended up backfiring as well. Any thoughts?

[robrix]

That’s fine :+1:

[robrix]

Okay, going to go ahead and merge. I’ve filed a couple of issues, and I’ll probably do a cleanup pass again, but it looks pretty good to me! :blush: Thank you so much!