Skip to content
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

Merged
merged 5 commits into from
Aug 24, 2023

Conversation

john-jam
Copy link
Contributor

@john-jam john-jam commented Aug 9, 2023

This PR updates the gcsfs implementation to support updates made in the Better double asterisks ** support pull request in the filesystem_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.

@ianthomas23
Copy link
Contributor

There is already a PR (#535) to add the derived tests to gcsfs.

@john-jam
Copy link
Contributor Author

john-jam commented Aug 9, 2023

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!

@ianthomas23
Copy link
Contributor

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.

@john-jam
Copy link
Contributor Author

john-jam commented Aug 9, 2023

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

@john-jam john-jam marked this pull request as ready for review August 16, 2023 01:54
@@ -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
Copy link
Contributor Author

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/")
Copy link
Contributor Author

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.

@john-jam
Copy link
Contributor Author

john-jam commented Aug 22, 2023

@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

@martindurant
Copy link
Member

rerunning

@martindurant martindurant merged commit 54eaa80 into fsspec:main Aug 24, 2023
5 checks passed
@john-jam john-jam deleted the better-double-asterisk-support branch August 24, 2023 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants