Enhance docs and tests for pred/succ/anc/desc #1405
Conversation
Pull Request Test Coverage Report for Build 14142482732Details
💛 - Coveralls |
There was a problem hiding this comment.
LGTM, thanks for documenting the current behaviors.
I am curious about what ..seealso is going to look on the docs, but it is a Sphinx directive and it is pretty appropriate for our situation. Our theme is patched but it should support it.
I left a minor Rust comment
src/digraph.rs
Outdated
| @@ -705,9 +715,8 @@ impl PyDiGraph { | |||
| let mut predec: Vec<&PyObject> = Vec::new(); | |||
| let mut used_indices: HashSet<NodeIndex> = HashSet::new(); | |||
| for pred in parents { | |||
| if !used_indices.contains(&pred) { | |||
| if used_indices.insert(pred) { | |||
There was a problem hiding this comment.
Actually, let's rewrite this to use this API with a match statement: https://docs.rs/hashbrown/latest/hashbrown/struct.HashSet.html#method.entry
That way the behavior is clearer. Your code and the existing codes are equivalent, your code saves a lookup but it is not obvious.
There was a problem hiding this comment.
Changed both predecessors/successors syntax.
Since the usage of this syntax is not consistent across the whole project, I'm going to consolidate it elsewhere.
b6865d5 to
04b6be6
Compare
It looks good, but the link is not rendered, so it's not useful. |
* Enhance docs and tests for pred/succ/anc/desc * Use match pattern in predecessors/successors
I realized the
ancestorsanddescendantsraise anIndexErrorif the provided node is not present in the directed graph. On the other handpredecessorsandsuccessors, which are quite similar methods, just return an empty list.I do not want to introduce a breaking change, so I just added a test pair to make sure this persists.
Also for the formatting, I discovered the docs website was using Furo which offers some nice details to distinguish different parts of a method documentation. Before we have lots of docs, we'll have to find some useful and decent style.