Fix GCSFS walk() method #561
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #561 +/- ##
==========================================
+ Coverage 86.08% 86.13% +0.05%
==========================================
Files 87 87
Lines 4972 4969 -3
Branches 793 792 -1
==========================================
Hits 4280 4280
+ Misses 563 560 -3
Partials 129 129 ☔ View full report in Codecov by Sentry. |
|
@selitvin @aleks-djuric Not sure what does the complaint from |
|
|
||
| yield path, directories, files | ||
| obj_path = obj['name'] | ||
| if obj_path == path: |
There was a problem hiding this comment.
This check still fails to catch the desired directory. obj_path has an extra '/' on the end which path does not.
There was a problem hiding this comment.
Thanks, I was able to reproduce, but for some reason only when I provided a directory that didn't contain files, only other directories.
|
@aleks-djuric ,If you are familiar with gcs nuances, perhaps you could help review this PR? |
|
I fixed this by just doing |
|
Either @megaserg incorporates the fix you propose (I think it's preferable since we'll keep only one PR for the issue), or create a separate PR and we can review and land that. @megaserg : we don't have tests for GCSFS and not sure if we can set them up easily. We will be able to land the PR regardless. |
|
@megaserg, as a last ask, since we don't have a proper unittest setup, can you please paste an example of how the walk function output looks like to the comment of this PR for a sanity check? |
| files.add(obj_path) | ||
|
|
||
| rel_files = sorted([posixpath.split(f)[1] for f in files | ||
| if f not in directories]) |
There was a problem hiding this comment.
What's the purpose of checking if f not in directories? Could there be an entry in files that is also in directories?
|
|
||
| yield path, directories, files | ||
| obj_path = obj['name'] | ||
| if obj_path.strip('/') == path.strip('/'): |
There was a problem hiding this comment.
Let's use more conservative rstrip.
|
|
||
| rel_files = sorted([posixpath.split(f)[1] for f in files | ||
| if f not in directories]) | ||
| rel_directories = sorted([posixpath.split(x[:-1])[1] |
There was a problem hiding this comment.
More self explanatory, IMO: x[:-1] -> x.rstrip('/').
| rel_directories = sorted([posixpath.split(x[:-1])[1] | ||
| for x in directories]) | ||
|
|
||
| yield path, rel_directories, rel_files |
There was a problem hiding this comment.
When path is invalid, the function still yields once. This does not match os.walk semantics which would not yield at all in that case.
Resolves #558