Skip to content

Commit

Permalink
Add missing nodes equality check in some tests
Browse files Browse the repository at this point in the history
  • Loading branch information
@easbar
easbar committed Jul 4, 2021
1 parent 0d4fa89 commit 20ba810
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 4 deletions.
7 changes: 5 additions & 2 deletions src/dijkstra.rs
View file
Expand Up @@ -344,10 +344,13 @@ mod tests {
weight: Weight,
nodes: Vec<NodeId>,
) {
let dijkstra_path = dijkstra.calc_path(&graph, source, target);
assert_eq!(
dijkstra.calc_path(&graph, source, target),
Some(ShortestPath::new(source, target, weight, nodes))
dijkstra_path,
Some(ShortestPath::new(source, target, weight, nodes.clone()))
);
// ShortestPath PartialEq does not consider nodes!

This comment has been minimized.

Sign in to view

Copy link
@easbar

easbar Jul 4, 2021

Author Owner

This is ugly. I think I now regret not including the nodes in ShortestPath PartialEq. @dabreegster do you have an opinion here? Should we add the nodes to the equality check? Or maybe remove ShortestPath PartialEq altogether?

This comment has been minimized.

Sign in to view

Copy link
@dabreegster

dabreegster Jul 4, 2021

Contributor

Yeah, this is strange. I would recommend just doing #[derive(PartialEq)] on ShortestPath, which will also compare the nodes. Even though multiple shortest paths might exist, a single ShortestPath struct represents exactly one of them, and fully comparing it to another ShortestPath is a useful thing to do -- the random test does exactly this. As long as the caller understands this, it should be fine.

This comment has been minimized.

Sign in to view

Copy link
@easbar

easbar Jul 4, 2021

Author Owner

Even though multiple shortest paths might exist, a single ShortestPath struct represents exactly one of them,

Yes, exactly.

and fully comparing it to another ShortestPath is a useful thing to do

Yes maybe useful, but not sure if it is absolutely necessary. I wonder what is better: Changing the implementation or removing it. Since it is a rather minor thing I think I'm not too afraid of a breaking change here.

This comment has been minimized.

Sign in to view

Copy link
@dabreegster

dabreegster Jul 4, 2021

Contributor

Since there's a subtlety here, maybe a safer API would be to remove PartialEq entirely and have an explicit fn is_same(&self, other: &ShortestPath) -> bool method that documents the edge case.

If you do make a change here, I think it's a breaking change from a semver perspective, so it warrants a new minor release. But as you say, it's a minor point, and if you don't know of anyone else using it, could probably just make a change.

This comment has been minimized.

Sign in to view

Copy link
@easbar

easbar Jul 4, 2021 •
edited

Author Owner

Ok, I don't think it's urgent but will keep this in mind / open an issue so maybe a change like this will be included when there will be other changes as well maybe. -> #31
assert_eq!(nodes, dijkstra_path.unwrap().get_nodes().clone());
assert_eq!(dijkstra.calc_weight(&graph, source, target), Some(weight));
}
}
7 changes: 5 additions & 2 deletions src/fast_graph_builder.rs
View file
Expand Up @@ -335,9 +335,12 @@ mod tests {
weight: Weight,
nodes: Vec<NodeId>,
) {
let dijkstra_path = calc_path(fast_graph, source, target);
assert_eq!(
calc_path(fast_graph, source, target),
Some(ShortestPath::new(source, target, weight, nodes))
dijkstra_path,
Some(ShortestPath::new(source, target, weight, nodes.clone()))
);
// ShortestPath PartialEq does not consider nodes!
assert_eq!(nodes, dijkstra_path.unwrap().get_nodes().clone(),);
}
}

0 comments on commit 20ba810

Please sign in to comment.