Skip to content
/ django Public

Fixed #30759 -- Made cache.delete() return whether it succeeded.#11883

Merged
felixxm merged 1 commit intodjango:masterfrom
MisterRios:30759/cache_delete_returns_results
Nov 14, 2019
Merged

Fixed #30759 -- Made cache.delete() return whether it succeeded.#11883
felixxm merged 1 commit intodjango:masterfrom
MisterRios:30759/cache_delete_returns_results

Conversation

@MisterRios
Copy link
Contributor

@MisterRios MisterRios commented Oct 8, 2019

I returned a tuple with (success, count), a la update_or_create but am not completely sure of this as the return value.

Native Memcache returns a simple TRUE or FALSE value, while native Redis returns an integer with the number of deleted keys, so it seems there is no set standard.

@charettes
Copy link
Member

charettes commented Oct 8, 2019

Hello @MisterRios thank you for your PR!

I think we should focus solely on .delete() -> bool for now given we can't reliably return a coherent results on all backends for delete_many because of mixed support. Support for delete_many could be added in a future ticket if we can come up with a coherent return type.

@MisterRios
Copy link
Contributor Author

MisterRios commented Oct 8, 2019

Hi @charettes! Thanks for the feedback.
It does simplify matters to just focus on delete(). Will do.

@charettes charettes changed the title Fixed #30759- Make cache.delete() return whether or not it suceeded Fixed #30759 -- Made cache.delete() return whether or not it suceeded. Oct 9, 2019
charettes
charettes reviewed Oct 9, 2019
View reviewed changes
@MisterRios MisterRios force-pushed the 30759/cache_delete_returns_results branch 2 times, most recently from b4e4f65 to 239bea9 Compare October 28, 2019 13:25
@MisterRios
Copy link
Contributor Author

MisterRios commented Oct 28, 2019

@charettes I went with option one. Though I will follow up with options two and three, it seemed the easiest way, since this issue has some history:
https://bugs.launchpad.net/python-memcached/+bug/471727
linsomniac/python-memcached#51

@felixxm felixxm self-assigned this Nov 14, 2019
@felixxm felixxm mentioned this pull request Nov 14, 2019
Closed
@felixxm felixxm changed the title Fixed #30759 -- Made cache.delete() return whether or not it suceeded. Fixed #30759 -- Made cache.delete() return whether it suceeded. Nov 14, 2019
@felixxm felixxm changed the title Fixed #30759 -- Made cache.delete() return whether it suceeded. Fixed #30759 -- Made cache.delete() return whether it succeeded. Nov 14, 2019
@felixxm felixxm force-pushed the 30759/cache_delete_returns_results branch from 239bea9 to 2f5b824 Compare November 14, 2019 10:06
@felixxm
Copy link
Member

felixxm commented Nov 14, 2019

@MisterRios Thanks for this patch 👍 I squashed commits, rebased from master, fixed release notes and documentation changes and pushed minor edits.

@felixxm felixxm force-pushed the 30759/cache_delete_returns_results branch from 2f5b824 to 94e44ce Compare November 14, 2019 10:08
@felixxm felixxm force-pushed the 30759/cache_delete_returns_results branch from 94e44ce to efc3e32 Compare November 14, 2019 10:14
@felixxm felixxm merged commit efc3e32 into django:master Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@felixxm felixxm felixxm left review comments

+1 more reviewer

@charettes charettes charettes left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@felixxm felixxm

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@MisterRios @charettes @felixxm