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

ensure error when deleting entire storage or deleting directory by delete method instead of deleteDirectory #1737

Closed
wants to merge 11 commits into from

Conversation

tinect
Copy link
Contributor

@tinect tinect commented Dec 26, 2023

I saw some custom adapters not managing these inputs. E.g. S3 and local adapter throw these exceptions.

I'd like to see these checks in the AdapterTestUtilities to ensure better quality.

@tinect tinect changed the title feat: add test cases to ensure error when deleting entire storage and deleting directory by delete method instead of deleteDirectory ensure error when deleting entire storage or deleting directory by delete method instead of deleteDirectory Dec 27, 2023
@tinect
Copy link
Contributor Author

tinect commented Jan 16, 2024

@frankdejonge what do you think about the issue?

@frankdejonge
Copy link
Member

Hi @tinect,

There a bunch of unrelated stuff in this PR, which makes it a bit messy. The package's design tends to not perform these kind of checks, but rather leave it up to the user to do this. The reason for this has multiple reasons. Firstly, this increases the amount of underlying calls significantly, making the default less performant. Secondly, which is kind of related to the first, the case where a directory has the same name as a file is very unlikely, as the majority of files have file extensions that wouldn't be in a directory name.

I'm happy to merge some of your other changes in this PR if you sent them individually, but I'm going to close this one. In case you feel like contributing more to this problem (which I'd be grateful for) please keep it to one thing per PR.

@tinect tinect deleted the feat/ensureDeleteException branch February 3, 2024 22:07
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