fix #69 and #70

[cecileane]

#69:

• arrange edge tuples by their vertex labels, to keep edge_data accessible after vertex deletion, which changes vertex codes.
• tests added
• documentation: added the need for labels to be comparable with <.

#70 for add_edge!:

• do nothing if the underlying graph already has an edge
• docstring updated to be explicit that this function cannot be used to update the data of an existing edge.

[gdalle]

Thank you @cecileane! I love people who open issues and immediately after that open PRs to fix them 😍 My favorite kind of users, basically no work for me 🏖️

[codecov]

Codecov Report

Merging #71 (197d134) into master (b769719) will not change coverage.
Report is 4 commits behind head on master.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master      #71   +/-   ##
=======================================
  Coverage   93.37%   93.37%           
=======================================
  Files           7        7           
  Lines         302      302           
=======================================
  Hits          282      282           
  Misses         20       20           

Files	Coverage Δ
src/dict_utils.jl	97.14% <100.00%> (ø)
src/directedness.jl	100.00% <100.00%> (ø)
src/graphs.jl	89.06% <100.00%> (+0.96%)	⬆️
src/weights.jl	100.00% <ø> (ø)

... and 1 file with indirect coverage changes

[gdalle]

We can further simplify by removing these code_i argument everywhere in the package

[cecileane]

I was thinking that too, but that meant small changes in many places. Done now!

[cecileane]

by the way: this simplification was done.
I also added a couple tests to check that the edge data was accessible and as expected.

[gdalle]

Could you maybe add a test specifically checking that the edge data remains the same after vertex deletion? Something similar to #69? Here you are only testing that arrange works, which is necessary but less robust

[gdalle]

Also, this is a breaking change, could you bump the major version number?

[cecileane]

Could you maybe add a test specifically checking that the edge data remains the same after vertex deletion? Something similar to #69? Here you are only testing that arrange works, which is necessary but less robust

I added tests in that direction, but I'm not sure I understood what you mean. The edge_data is modified in place (entries removed) so the main concern was about the keys, not the values (edge data).

[bramtayl]

I'm not sure I like the API of doing nothing if the vertex already exists. I think it makes more sense to change the data if the user requests it?

[cecileane]

I'm not sure I like the API of doing nothing if the vertex already exists.

Do you mean if the edge already exists, and we try to add it again with add_edge!, possibly with different data than the currently existing data for that edge?

I don't have any strong opinion. It might be "safe" to not overwrite existing data with add_edge! simply because "add" suggests that the edge is absent before being added. In that case, modifying an edge data should be done with graph[label1, label2] = data. But this API choice is your choice, not mine. I'm happy to update the PR either way.

[gdalle]

The edge_data is modified in place (entries removed) so the main concern was about the keys, not the values (edge data).

I just meant that we should check we are still able to access the right data for each edge after removal.

In that case, modifying an edge data should be done with graph[label1, label2] = data

Maybe add_edge! should just error when the edge already exists

[bramtayl]

Do you mean if the edge already exists, and we try to add it again with add_edge!, possibly with different data than the currently existing data for that edge?

Yup

I don't have any strong opinion

Me neither

[cecileane]

Maybe add_edge! should just error when the edge already exists

I think add_edge!(::MetaGraph) should follow Graphs' choice where Graphs.add_edge!(::Graph) returns false if the edge addition failed (because it existed already, for example), with no error.

[gdalle]

Okay then, if the edge is already present, let's modify the existing data binding. When multigraphs are supported, there might be a different answer

See #68

[cecileane]

done with b98373a: to make add_edge! modify existing edge data.