allow vertices to have weights and attributes

[tobikris]

This adds the possibility for vertices to have weights and attributes - just like edges. It solves #32 and is based on the idea from the issue. Graphs store the vertices' properties in a map based on the hash value.

[dominikbraun]

Thanks! Supporting functional options in AddVertex makes sense and is consistent with the edge API, so this is a nice suggestion.

I'll take a look soon. One thing that is yet to be clarified is how these changes affect #50, and whether this PR or #50 has precedence. What do you think, @geoah?

[geoah]

Owh this looks great!

@dominikbraun happy to rebase #50 once this is merged, this seems like it's closer to merging.

Small suggestion, feel free to ignore it: I wonder if it would make any sense to make the VertexProperties(hash K) (VertexProperties, error) (or even the Vertex() one) to return both the vertex and the properties. ie VertexWithProperties(hash K) (T, VertexProperties, error) so that there is an alternative for asking for both instead of having to call two methods. For most of my usecases I don't think I'd ever need just the properties, but that might be a niche.

[tobikris]

Small suggestion, feel free to ignore it: I wonder if it would make any sense to make the VertexProperties(hash K) (VertexProperties, error) (or even the Vertex() one) to return both the vertex and the properties. ie VertexWithProperties(hash K) (T, VertexProperties, error) so that there is an alternative for asking for both instead of having to call two methods. For most of my usecases I don't think I'd ever need just the properties, but that might be a niche.

Thanks. I thought about this as well and decided against returning the properties as well to remain somewhat compatible. But I would be fine with implementing it in any of the mentioned ways. Your call, @dominikbraun

[dominikbraun]

Small suggestion, feel free to ignore it: I wonder if it would make any sense to make the VertexProperties(hash K) (VertexProperties, error) (or even the Vertex() one) to return both the vertex and the properties. ie VertexWithProperties(hash K) (T, VertexProperties, error) so that there is an alternative for asking for both instead of having to call two methods. For most of my usecases I don't think I'd ever need just the properties, but that might be a niche.

Thanks. I thought about this as well and decided against returning the properties as well to remain somewhat compatible. But I would be fine with implementing it in any of the mentioned ways. Your call, @dominikbraun

Good question. I think both VertexProperties(K) (VertexProperties, error) and VertexWithProperties(K) (T, VertexProperties, error) would be fine.

However, when using a library, I personally don't like having to handle an error after each and every line. This is why I, as a user, would probably prefer this:

vertex, properties, err := g.VertexWithProperties(hash)
if err != nil {
    panic(err)
}

Over this:

vertex, err := g.Vertex(hash)
if err != nil {
    panic(err)
}

properties, err := g.VertexProperties(hash)
if err != nil {
    panic(err)
}

Because retrieving the vertex properties without the vertex itself should be more of an edge case, I consider this a viable approach. One counterargument would be that VertexWithProperties looks like a primitive getter function and hides some complexity, but retrieving a vertex is an O(1) operation. Even if the user ultimately only wants to get the properties, the lookup of the vertex is unlikely to be a performance killer.

That's why I probably would prefer VertexWithProperties (or VertexAndProperties?), but either one is fine.

[tobikris]

I like the VertexWithProperties version most and changed the code accordingly.

[dominikbraun]

I've slightly changed the test setup for the AddVertex tests so that they are consistent with the AddEdge tests.

[tobikris]

Thanks!

In https://github.com/dominikbraun/graph/commit/1941d0eb2f43aa37ea63feeb7ac905a465be2045 in line 12 the method name is incorrect. Maybe you could fix this?

[dominikbraun]

Thanks!

In 1941d0e in line 12 the method name is incorrect. Maybe you could fix this?

Good catch, thanks!

[dominikbraun]

This change has been released with graph v0.13.0.