Rewrite the pathmap.Tree
#637
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #637 +/- ##
==========================================
- Coverage 98.12% 98.12% -0.01%
==========================================
Files 437 437
Lines 36749 36664 -85
==========================================
- Hits 36061 35976 -85
Misses 688 688
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #637 +/- ##
==========================================
- Coverage 98.16% 98.16% -0.01%
==========================================
Files 476 476
Lines 38070 37985 -85
==========================================
- Hits 37372 37287 -85
Misses 698 698
Flags with carried forward coverage won't be shown. Click here to find out more.
This change has been scanned for critical changes. Learn more |
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #637 +/- ##
==========================================
- Coverage 98.12% 98.12% -0.01%
==========================================
Files 437 437
Lines 36749 36664 -85
==========================================
- Hits 36061 35976 -85
Misses 688 688
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #637 +/- ##
==========================================
- Coverage 98.12% 98.12% -0.01%
==========================================
Files 437 437
Lines 36749 36664 -85
==========================================
- Hits 36061 35976 -85
Misses 688 688
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Swatinem
force-pushed
the
swatinem/rewrite-pathfixer
branch
from
98f6356
to
c5b3b51
Compare
|
|
||
| def insert(self, path: str): | ||
| # the path components, in reverse order | ||
| components = reversed(path.split("/")) |
There was a problem hiding this comment.
I think it'd be a good idea to add a comment explaining why we process the path in reverse as a docstring on the Tree class. It took me a while to remember why and I think it'd be very useful for devs seeing this for the first time. Something like: "this is processed in reverse because we're trying to match paths that have differing parent dirs at the start that at some point converge to matching names for the parent dirs. For ex: dir1/dir2/file1.txt and tmpdir1/tmpdir2/dir1/dir2/file1.txt.
There was a problem hiding this comment.
Thanks for the suggestion. I added a docstring explaining the internal structure with an example.
This mostly rewrites the `Tree`, making the following changes and optimizations: - Uses a real `Node` struct with children and terminals, instead of abusing special keys for it. - Avoids constructing needless non-terminal strings for all intermediate nodes. - Constructs the tree directly iteratively, instead of creating a parallel tree and merging recursively. - Switches from recursion to iteration for `_drill`. It should be possible to also avoid recursion in lookup, but with a bit more effort. This should primarily improve construction performance and improve memory usage, which was the primary pain points with the previous implementation.
Swatinem
force-pushed
the
swatinem/rewrite-pathfixer
branch
from
9287600
to
2661202
Compare
This mostly rewrites the
Tree, making the following changes and optimizations:Nodestruct with children and terminals, instead of abusing special keys for it._drill. It should be possible to also avoid recursion in lookup, but with a bit more effort.This should primarily improve construction performance and improve memory usage, which was the primary pain points with the previous implementation.