Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add error structs to give more information on errors #156

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

gordon-klotho
Copy link

Adds the following error structs:

  • VertexAlreadyExistsError[K, T]
  • VertexNotFoundError[K]
  • EdgeAlreadyExistsError[K]
  • EdgeNotFoundError[K]
  • VertexHasEdges[K]
  • EdgeCausesCycleError[K]

This may be breaking if users check err == instead of errors.Is for specific errors (such as err == ErrVertexNotFound and not errors.Is(err, ErrVertexNotFound). Depending on versioning strategy and definition of a breaking change, this may warrant a new major version.

This change also fixes a few issues I found in memoryStore:

  • AddEdge did not conform to the Store interface: "If either vertex doesn't exit, ErrVertexNotFound should be returned for the respective vertex"
  • UpdateEdge had a data race in which the edge could have changed between the s.Edge call and the setting of the edges due to not holding onto the lock.
  • CreatesCycle was not read locking before accessing s.inEdges

To let the store handle error cases when it is part of its interface definition, undirected and directed:

  • AddEdge don't check the vertices for existence
  • RemoveEdge don't check edge existence

And a few minor changes I made while I was there:

  • Use ListEdges instead of AdjacencyMap for Size, saves some allocations and another pass through the edges to create the map
  • undirected copy directed's createsCycle method to delegate to the store's version for a more performant implementation

@dominikbraun dominikbraun self-requested a review October 13, 2023 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant