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 #769

Merged
merged 4 commits into from
Aug 25, 2023

Conversation

john-jam
Copy link
Contributor

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

This PR updates the s3fs 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/15915309957

@john-jam john-jam force-pushed the better-double-asterisk-support branch from 2f79237 to a9044a9 Compare August 16, 2023 01:46
@john-jam john-jam marked this pull request as ready for review August 16, 2023 01:55
with fsspec.open("mine/oi", "rb") as f:
with fsspec.open(
"s3://mine/oi", "rb", client_kwargs={"endpoint_url": endpoint_uri}
) as f:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is unrelated to glob double asterisk but I think it was wrong. It tested the LocalFileSystem implementation instead of the S3 one.


assert s3.ls(test_bucket_name) == s3.find(
ls_output = s3.ls(test_bucket_name)
assert sorted(ls_output + [test_bucket_name]) == s3.find(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

find now returns the current folder as well.

@@ -969,6 +969,8 @@ async def _ls(self, path, detail=False, refresh=False, versions=False):
for o in files
if o["name"].rstrip("/") == path and o["type"] != "directory"
]
if not files:
raise FileNotFoundError(path)
Copy link
Contributor Author

@john-jam john-jam Aug 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should raise FileNotFoundError when no files are found to be consistent.

@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

@john-jam
Copy link
Contributor Author

@martindurant Thanks for the rerun. Merging this PR should then fix the filesystem_spec latest master CI: https://github.com/fsspec/filesystem_spec/actions/runs/5968251389/job/16191678409

@martindurant martindurant merged commit b1d9880 into fsspec:main Aug 25, 2023
5 checks passed
@john-jam john-jam deleted the better-double-asterisk-support branch August 29, 2023 07:58
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.

2 participants