Making 'addNode' an idempotent function, no more exceptions!

[recursion-ninja]

Throwing an exception when an existing node is added to the "graph" makes recursive construction via the PrintDot type class fragile. It seems more intuitive that if a node is already in the graph, it's information should be merged rather than an exception being thrown.

[ivan-m]

I'm in two minds about this pull request.

Graph libraries typically don't allow you to add the same node over and over again, and I think it's possible that adding this change in may result in someone accidentally having unexpected behaviour because their code happens to try and add the same node twice; an exception isn't ideal but it can alert the user (albeit at runtime) as to what they're doing.

[recursion-ninja]

Could you provide some examples of libraries that throw exceptions when duplicate nodes are added to a graph? Throwing exceptions outside of IO, seems to me, to be the opposite of idiomatic Haskell.

From a mathematical standpoint, a graph G is a set of vertices V and a set of edges E. If we were to add a vertex v twice to the vertex set V of a graph G, I would expect the same behavior of adding an element twice to a set: idempotence.

For example, if we let f equal the following expression:

f :: Graph -> Graph
f = addNode n

I would expect the subsequent referential transparencies to hold:

f graph === f (f graph) === f (f (f graph)) === f (f (f (f graph))) ...

I was quite taken a back when the exception was thrown when testing our latest build. I actually believe makes the library unusable in a pure context. Furthermore, the exception returns no actionable information to diagnose the issue. An Either or Maybe return value would signify the failibility of the operation. I would expect idempotence, but if you want to keep a failable operation I would suggest changing the type to Maybe or Either so library users can recover from the failure in a pure context. However, making the failablitiy explicit in the return type would require percolating the failure conditions up through many other functions in the library, or having the other functions handle the failure conditions. That general yuckiness is why I prefer, and many other Haskell libraries implement, idempotent functions if possible to avoid exceptions or explicit failure results.

[ivan-m]

Actually, in Dot multiple statements for a node accumulates attributes. So I think that's enough prior art for this. Can you just document that behaviour please?

[ivan-m]

And you already have :D

[recursion-ninja]

I didn't add an entry in the changelog.md file though. You might want to do that before the next hackage upload.