Fix issue 46 by adding metadata g to all returned edge objects

[jafingerhut]

If we modify find-edges-impl, it will also cause all of the functions listed below to return edges with metadata g, since they all end up calling find-edges-impl:

• find-edge-impl
• find-edges
• find-edge
• get-edge

[Engelberg]

Thanks. Since this would change the function's return type from a set to a seq, I need to do some double-checking to be sure that's not a problem.

[Engelberg]

For background, I think the reason I didn't bother with attaching the metadata here is that originally, that was sort of a hack so that I when passed graphs to loom algorithm's, which frequently destructure an edge as [src dest weight], there would be a way to pull the weight out of an edge for the destructuring. Since it was only really meant to work in the context of loom algorithms, the find-edges protocol, which is unique to ubergraph, didn't seem like something to address.

But, now there are a couple other places (like build-graph) where adding an Edge object to a graph that came from another graph feels most natural if you bring the attributes along with it. I've never personally used that feature (adding Edge object from one graph to another), but it seems like something potentially worth supporting. So that's why I would agree that this is a problem I'd like to address. Otherwise, I should probably scrap that feature.

[Engelberg]

Looking at it, I guess it doesn't currently promise a set return value in all possible code branches.

[jafingerhut]

I don't actually have a use case in mind for taking edge objects from one graph and adding them directly to another. There seem to be at least a few subtleties with using edges obtained from one graph in calls to others (as this issue demonstrates https://github.com/Engelberg/ubergraph/issues/48), but you are right that for build-graph perhaps it makes most sense, assuming that the new graph get new UUIDs for edges added to it.

That said, it is certainly no problem for me to close this if we find that the changes are not useful.

[Engelberg]

After doing some soul-searching on this issue over the past couple of weeks, I've come to the conclusion that it's a bad idea for end users to expect the edges to have graph and therefore attribute info in the edge's metadata. This creates the wrong mental model, where users will imagine the attributes belong to the edge, rather than correctly programming to the abstraction that the attributes are stored in the graph itself.

It's still going to be necessary to leave that metadata hack in place on the protocols that loom uses, so that those algorithms can get at the weight via destructuring. But I don't want this to be a feature of the ubergraph API.

My current plan is:

1. Revert the changes I made in the last update where I expanded the set of functions where you could pass Edge objects instead of edge descriptions.
2. Throw errors in the places where Edge objects are not accepted.
3. Think about whether it makes sense to remove Edge objects as "inits" for build-graph (ideally would like to take it out now, but it may still be necessary as an implementation detail for being able to use a whole other graph as an init, which is something I do want to support; also, want to be careful about breaking existing code).
4. Add some convenience functions like maybe node-with-attrs and edge-with-attrs that return [node attribute-map] and [src dest attribute-map] vectors that can be easily destructured and can be used conveniently as inits in build-graph.

Will be working on this over the next couple of days and will then cut a release with these changes and the other pull request I just merged, so further comments welcome.

[Engelberg]

I have completed the changes to code and README to prevent edge objects from being used in graph constructors, and added node-with-attrs and edge-with-attrs. Take a look and let me know what you think, and then I'll push this as version 0.7.0.