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

BREAKING implement (*Backend).Delete method per #1 #6

Merged
merged 4 commits into from
Jul 20, 2022
Merged

Conversation

ahouene
Copy link
Contributor

@ahouene ahouene commented Jul 13, 2022

(*Backend).Delete has been implemented for all three existing backends. Corresponding tests have been added.

backends/fs/fs.go Outdated Show resolved Hide resolved
@wojas
Copy link
Member

wojas commented Jul 18, 2022

One remaining design issue is what to do if we could not delete the object because it does not exist.

The current implementation returns ErrNotExist in such case, but I think I would prefer to not return any error in such a case. All the other methods are idempotent, allowing you to repeat the same call without getting different results. Returning ErrNotExist breaks this pattern. HTTP DELETE is also idempotent.

Not returning an error in this case also makes the API easier to use. I think most code that consumes this API would end up checking for this specific error and ignoring it. I cannot think of a case where getting an ErrNotExist is useful. If you really care if it existed, you could do a List before the call.

@ahouene
Copy link
Contributor Author

ahouene commented Jul 19, 2022

Good point and insight, I'm fixing this!

methods idempotency was broken by the introduction of the Delete method,
this is no longer the case
Copy link
Member

@wojas wojas left a comment

Choose a reason for hiding this comment

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

LGTM! We will need to release a v0.2.0 with this change.

plugins.go Outdated Show resolved Hide resolved
Explain no error will be returned on missing key

Co-authored-by: wojas <[email protected]>
@ahouene ahouene merged commit 9883db0 into main Jul 20, 2022
@ahouene ahouene mentioned this pull request Jul 20, 2022
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