-
Notifications
You must be signed in to change notification settings - Fork 146
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Better double asterisk support #572
Better double asterisk support #572
Conversation
There is already a PR (#535) to add the derived tests to gcsfs. |
Oh sorry, I didn't see it. Do you plan to merge it soon? Having those is really convenient to test the repo changes! |
I'd love to merge it soon, but there are 3 test failures that I haven't fixed yet. |
The derived tests succeed on my branch with my updates here (I temporarily updated the fsspec CI to clone my gcsfs branch): https://github.com/john-jam/filesystem_spec/actions/runs/5807521471/job/15742515177 |
@@ -1294,7 +1294,7 @@ async def _find( | |||
|
|||
if maxdepth: | |||
# Filter returned objects based on requested maxdepth | |||
depth = path.count("/") + maxdepth | |||
depth = path.rstrip("/").count("/") + maxdepth |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The depth is wrong when there is a trailing slash in the source path. We could also strip it at the very beginning of the _find
method (the tests also works).
@@ -274,7 +274,7 @@ def test_gcs_glob(gcs): | |||
fn = TEST_BUCKET + "/nested/file1" | |||
assert fn not in gcs.glob(TEST_BUCKET + "/") | |||
assert fn not in gcs.glob(TEST_BUCKET + "/*") | |||
assert fn in gcs.glob(TEST_BUCKET + "/nested/") | |||
assert fn not in gcs.glob(TEST_BUCKET + "/nested/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing slashes in globs now returns only the directories.
@martindurant @ianthomas23 Since the upstream PR has been merged, the CI should succeed now. EDIT: there's an import error for common glob edge case tests. This PR should fix it: fsspec/filesystem_spec#1339 |
rerunning |
This PR updates the
gcsfs
implementation to support updates made in the Better double asterisks ** support pull request in thefilesystem_spec
repo.Successful run: https://github.com/fsspec/filesystem_spec/actions/runs/5847462518/job/15915309794
@ianthomas23 Let me know if you prefer to merge your PR with
derived test
and I'll remove my updates regarding this.