normalize edges to use Edge[K] rather than both Edge[K] and Edge[T], … #147
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…also addressed storing duplicate edge properties within the memory store.
| @@ -131,25 +131,15 @@ func (d *directed[K, T]) AddEdgesFrom(g Graph[K, T]) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (d *directed[K, T]) Edge(sourceHash, targetHash K) (Edge[T], error) { | |||
| func (d *directed[K, T]) Edge(sourceHash, targetHash K) (Edge[K], error) { | |||
There was a problem hiding this comment.
Graph.Edge returning a T is by design! The idea is that you obtain a vertex that you can directly operate on. For example, when storing instances of type City:
route, _ := g.Edge("sunnyvale", "mountain-view")
fmt.Printf("Weather at %s: %s\n", route.Target.Name, route.Target.LoadWeatherInfo())Requiring the user to look up the cities by their IDs first would kind of defeat the purpose of storing a T any in the first place, because then pure IDs would work as well. The memory overhead of returning these instances should be neglectable.
| @@ -232,6 +232,21 @@ type EdgeProperties struct { | |||
| Data any | |||
| } | |||
|
|
|||
| func (p *EdgeProperties) Clone() EdgeProperties { | |||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…also addressed storing duplicate edge properties within the memory store which addresses issue #110.
Currently the codebase seems to be a bit fragmented, in that there are many instances where edges are defined using Edge[K] and others using Edge[T]. While this isn't necessarily a big deal when K and T are the same type, it can become an issue when they're not. I opted to go with Edge[K] instead of Edge[T], because I figured it would be easier to guarantee the references / pointers were correct using the keys/hashes than with the vertices directly when passing them from the store to the graph. That said, if you think Edge[T] is the better choice I'd be interested to hear your ideas on how to handle that.
It should be noted, if it isn't already obvious that this will likely be a breaking change for some users of the library.