Refactor path_to #3875
Refactor path_to #3875
Conversation
Codecov ReportBase: 100.00% // Head: 100.00% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## main #3875 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 179 179
Lines 13800 13811 +11
=========================================
+ Hits 13800 13811 +11
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
|
||
| # Are we in the right ballpark? | ||
| # NB: Comparisons have a higher precedence than `not`. | ||
| if not self.get_start_loc() <= other.get_start_loc() <= self.get_end_loc(): | ||
| return None | ||
| return [] |
There was a problem hiding this comment.
I have sometimes wondered if this result should be indicated a different way. It could be an error in some situations.
Also, does this result become ambiguous now that the last step is removed? It could be interpreted in two ways:
- There is no path to
otherfromself. otheris an immediate child ofself.
There was a problem hiding this comment.
So my interpretation of this is that it's ok for [] to be the only result and not require an additional None result. The reason is that [] should always be interpreted as other is not a descendent of self. That interpretation would include other is self, because something is not a descendent of itself.
I'll update the docstring to be clearer on that - but does that make sense?
There was a problem hiding this comment.
I've updated the docstring with the explanation:
The result of this should be interpreted as *the path from `self` to `other`*.
If the return value is `[]` (an empty list), that implies there is no path
from `self` to `other`. This would include the case where the two are the same
segment, as there is no path from a segment to itself.Does that make more sense?
There was a problem hiding this comment.
Oh, I see -- if other is an immediate child, it returns a path containing self. The code is definitely good. In mathematical terms, the path we're returning would be described as a "half open interval", which we could mention in the docs as [self, other). I think it would be cool to add that additional explanation in the docstring, because it's formal and precise about what's being returned. Sometimes words are not enough. 😛
There was a problem hiding this comment.
@barrywhart - I've added some better tests to spell it out better and a not in the docstring. Reckon that's better?
Co-authored-by: Barry Hart <barrywhart@yahoo.com>
Co-authored-by: Barry Hart <barrywhart@yahoo.com>
This is another relatively separable part of #3847. It's a useful development alone and can be merged before the rest. That will also simplify the review of the other PR.
This changes the behaviour of
BaseSegment.path_to().Previously it would return a
listof segments. The first element of that list would beselfand the last would be the segment passed asother. The others would be the segments between.I noticed that many uses of
path_toalways cut the last element (i.e. slice[:-1]), and that there are use cases where we want to know more than just the lineage, but also where in the parent each child is.This changes
path_toto instead:listofPathStep(adataclass), which combines the segment with a newindexwhich states where in the parent the child was found. This is useful for understanding whether the segment was at the start or end of a parent.other(i.e. the last element previously returned). This is partly because it was not often used, partly because anyone callingpath_toalready has access to that and mostly because it doesn't make sense to ask what index something is within itself.