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

Add special checks for if_none_match("*") #5505

Closed
Xuanwo opened this issue Jan 3, 2025 · 2 comments · Fixed by #5506
Closed

Add special checks for if_none_match("*") #5505

Xuanwo opened this issue Jan 3, 2025 · 2 comments · Fixed by #5506
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@Xuanwo
Copy link
Member

Xuanwo commented Jan 3, 2025

          Oh no worries. I think the API is good, perhaps the only improvement having the above in mind would be to only allow `.if_none_match("*")` and error out for any other argument.

Alternatively, perhaps the error message could state that if the intention is to use .if_none_match("*") then .if_not_exists(true) is an adequate replacement for S3.

Originally posted by @gruuya in #5504 (comment)

@Xuanwo
Copy link
Member Author

Xuanwo commented Jan 3, 2025

We can internally convert if_none_match("*") to if_not_exists if the underlying services support if_not_exists. This can prevent many users from being confused about the behavior.

Or, perhaps we should raise a clear and helpful error instead of attempting to convert it, ensuring our behavior remains consistent rather than relying on too many workarounds.

@gruuya
Copy link
Contributor

gruuya commented Jan 3, 2025

I thought about it a bit, one option would be to go with enabling if-none-match-* only for S3 like here #5506.

If you like that approach I can go ahead and extend the PR with some tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants