fix(syncer): [issue #42] Remove redundant path slices in file paths and ancestor root#44
fix(syncer): [issue #42] Remove redundant path slices in file paths and ancestor root#44j-archit wants to merge 1 commit intotkhyn:developfrom
Conversation
tkhyn
left a comment
tkhyn
left a comment
There was a problem hiding this comment.
Hi, thanks for the PR and sorry for the very late review. See my comments below!
| if add_path: | ||
| left.add(path) | ||
| anc_dirs = re_path[:-1].split('/') | ||
| anc_dirs = re_path.split('/') |
There was a problem hiding this comment.
I really can't remember why the [:-1] was added here. This dates from when I completely rewrote / adapted dirsync from the original author's code. It was possibly to remove the trailing slash for directories, and I may never have hit this line with re_path being a file. However, to safe-guard against adding too many items to left, shouldn't we either use re_path.rstrip("/").split("/") or add a if not ad: continue in the for loop that follows?
| anc_dirs = re_path[:-1].split('/') | ||
| anc_dirs = re_path.split('/') | ||
| anc_dirs_path = '' | ||
| for ad in anc_dirs[1:]: |
There was a problem hiding this comment.
I guess we have to strip the root directory as we don't want it to appear in the left list. Same, this dates from when the module was completely rewritten back in 2014 and it looks like I did not write enough comments! Does stripping the root create issues? If yes, I would be happy to see a failing test case added to the test suite.
2 participants
Removes possibly redundant path and ancestor root slicing as mentioned in #42.
Test results:
Before
After
