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

missing_ok kwarg for unlink diverges from pathlib defaults #231

Open
pjbull opened this issue May 31, 2022 · 2 comments
Open

missing_ok kwarg for unlink diverges from pathlib defaults #231

pjbull opened this issue May 31, 2022 · 2 comments
Labels
question Further information is requested

Comments

@pjbull
Copy link
Member

pjbull commented May 31, 2022

For the unlink function, missing_ok=True by default. The default in pathlib is missing_ok=False.

This has always been the behavior, but as of #230 it is made explicit.

If we want to follow pathlib, we'll need a deprecation cycle to warn folks.

Original context below.


Yeah, I'm on the fence about this diverging from pathlib defaults. IMO this is the nicer default, but in general we use pathlib as a pretty strict guide. @jayqi any thoughts here?

I guess if we want to follow pathlib defaults here at some point we need to go through a deprecation warning cycle first.

Originally posted by @pjbull in #230 (comment)

@karolzlot
Copy link
Contributor

Current default is more network efficient, so if this is changed to default from pathlib then documentation should have some warning information about this inefficiency.


I don't like pathlib default in this case, for me it seems unpythonic, but still its probably better choice to not diverge from pathlib.

@jayqi
Copy link
Member

jayqi commented Jun 22, 2023

I'm generally in favor of matching pathlib's default.

Regarding efficiency, I think that would depend on the particular backend implementation. I don't see any extra network calls in the case that missing_ok=False for any of our implementations.

def _remove(self, cloud_path: S3Path, missing_ok: bool = True) -> None:
file_or_dir = self._is_file_or_dir(cloud_path=cloud_path)
if file_or_dir == "file":
resp = self.s3.Object(cloud_path.bucket, cloud_path.key).delete(
**self.boto3_list_extra_args
)
if resp.get("ResponseMetadata").get("HTTPStatusCode") not in (204, 200):
raise CloudPathException(
f"Delete operation failed for {cloud_path} with response: {resp}"
)
elif file_or_dir == "dir":
# try to delete as a direcotry instead
bucket = self.s3.Bucket(cloud_path.bucket)
prefix = cloud_path.key
if prefix and not prefix.endswith("/"):
prefix += "/"
resp = bucket.objects.filter(Prefix=prefix, **self.boto3_list_extra_args).delete(
**self.boto3_list_extra_args
)
if resp[0].get("ResponseMetadata").get("HTTPStatusCode") not in (204, 200):
raise CloudPathException(
f"Delete operation failed for {cloud_path} with response: {resp}"
)
else:
if not missing_ok:
raise FileNotFoundError(
f"Cannot delete file that does not exist: {cloud_path} (consider passing missing_ok=True)"
)

def _remove(self, cloud_path: AzureBlobPath, missing_ok: bool = True) -> None:
file_or_dir = self._is_file_or_dir(cloud_path)
if file_or_dir == "dir":
blobs = [
b.blob for b, is_dir in self._list_dir(cloud_path, recursive=True) if not is_dir
]
container_client = self.service_client.get_container_client(cloud_path.container)
container_client.delete_blobs(*blobs)
elif file_or_dir == "file":
blob = self.service_client.get_blob_client(
container=cloud_path.container, blob=cloud_path.blob
)
blob.delete_blob()
else:
# Does not exist
if not missing_ok:
raise FileNotFoundError(f"File does not exist: {cloud_path}")

def _remove(self, cloud_path: GSPath, missing_ok: bool = True) -> None:
file_or_dir = self._is_file_or_dir(cloud_path)
if file_or_dir == "dir":
blobs = [
b.blob for b, is_dir in self._list_dir(cloud_path, recursive=True) if not is_dir
]
bucket = self.client.bucket(cloud_path.bucket)
for blob in blobs:
bucket.get_blob(blob).delete()
elif file_or_dir == "file":
bucket = self.client.bucket(cloud_path.bucket)
bucket.get_blob(cloud_path.blob).delete()
else:
# Does not exist
if not missing_ok:
raise FileNotFoundError(f"File does not exist: {cloud_path}")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants