Re: [Django] #12982: Add get_or_set method to cache API
Django
-------------------------------------+-------------------------------------
Reporter: Alex | Owner: snow0x2d0
Type: New | Status: new
feature | Component: Core (Cache system)
Milestone: 1.4 | Severity: Normal
Version: SVN | Keywords: cache
Resolution: | Has patch: 1
Triage Stage: Ready for | Needs tests: 0
checkin | Easy pickings: 0
Needs documentation: 0 |
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------
Changes (by ptone):
* status: assigned => new
* owner: jmeed => snow0x2d0
* ui_ux: => 0
* easy: => 0
* stage: Accepted => Ready for checkin
Comment:
patch applies to rev 16843
Docs are clear
tests look pretty thorough
--
Ticket URL: <https://code.djangoproject.com/ticket/12982#comment:12>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: Alex | Owner: snow0x2d0
Type: New | Status: new
feature | Component: Core (Cache system)
Milestone: 1.4 | Severity: Normal
Version: SVN | Keywords: cache
Resolution: | Has patch: 1
Needs documentation: 0 | Easy pickings: 0
Patch needs improvement: 1 |
UI/UX: 0 |
-------------------------------------+-------------------------------------
Changes (by Alex):
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
Moving back to accepted, there are a few issues: 1) in the docs,
`get_or_set()` should show its return values, 2) `get_or_set()` should
*only* take a callable second argument, it's too easy to get the
parameters wrong otherwise, 3) it should handle when `add()` raises an
error (which it will if the key is already set).
Thanks for working on this ticket!
--
Ticket URL: <https://code.djangoproject.com/ticket/12982#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Alex | Owner: snow0x2d0
Component: Core (Cache system) | Version: master
Severity: Normal | Resolution:
Keywords: cache | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by mumino):
* cc: mumino (added)
Comment:
I made a patch based on snow0x2d0's patch.
my proposal is a little different signature for get_or_set:
{{{
cached_data, is_set = cache.get_or_set(key, default, timeout=None,
version=None)
}}}
maybe we shouldn't force second parameter be callable. I prefer
{{{
cache.get_or_set(score, 0}
}}}
to
{{{
cache.get_or_set(score, lambda: 0}
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/12982#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: Alex | Owner: snow0x2d0
Type: New feature | Status: new
Component: Core (Cache system) | Version: master
Severity: Normal | Resolution:
Keywords: cache | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by snow0x2d0):
Wow, I completely missed the updates to this apparently, my apologizes.
I created two topic github branches:
https://github.com/snow0x2d0/django/tree/ticket_12982
https://github.com/snow0x2d0/django/commit/07d441763ce2fbae801cf8fe024aa8ab72e204e9
https://github.com/snow0x2d0/django/tree/ticket_12982_b
https://github.com/snow0x2d0/django/commit/5942333a7994e1a33e050d85da03dfa30ae22b12
They both address two of Alex's concerns. Documentation has been updated
to show the return values and an additional condition has been added to
catch when add fails if a key exists. From looking over the other backend
code it looks like in most cases add() will return False if a key exists
already (this is the case for both memcache backends). In the case of
database backend an add() with an existing key simply functions like a
set().
Personally I don't know that there is likely to be confusion over the
order of arguments. The order of parameters is just like the get()
function and also follows the function name (namely get, then set). That
being the case the primary difference between the two branches above is
that the ticket_12982_b branch the second parameter _may_ be callable and
in the ticket_12982 branch the second argument _must_ be callable (and a
typeerror is thrown if not).
Both branches have been rebased to the current master commit (as of about
6am CST 10-29-12) and all tests pass. I can submit a proper pull request
for either once approved.
--
Ticket URL: <https://code.djangoproject.com/ticket/12982#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: Alex | Owner:
Component: Core (Cache system) | Status: assigned
Severity: Normal | Version: master
Keywords: cache | Resolution:
Has patch: 1 | Triage Stage: Accepted
Needs tests: 0 | Needs documentation: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by berkerpeksag):
* owner: snow0x2d0 => berkerpeksag
* needs_better_patch: 1 => 0
* status: new => assigned
Comment:
https://github.com/django/django/pull/3609
--
Ticket URL: <https://code.djangoproject.com/ticket/12982#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: Alex | Owner:
Type: New feature | berkerpeksag
Component: Core (Cache system) | Status: assigned
Severity: Normal | Version: master
Keywords: cache | Resolution:
Has patch: 1 | Triage Stage: Accepted
Needs tests: 0 | Needs documentation: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by timgraham):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/12982#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: Alex | Owner:
Type: New feature | berkerpeksag
Component: Core (Cache system) | Status: assigned
Severity: Normal | Version: master
Keywords: cache | Resolution:
Has patch: 1 | Triage Stage: Accepted
Needs tests: 0 | Needs documentation: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by berkerpeksag):
* needs_better_patch: 1 => 0
Comment:
I've just updated the PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/12982#comment:19>
Django
-------------------------------------+-------------------------------------
Reporter: Alex | Owner:
Type: New feature | Status: assigned
Component: Core (Cache system) | Version: master
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by timgraham):
* stage: Accepted => Ready for checkin
Comment:
Looks good, pending a few cosmetic tweaks.
--
Ticket URL: <https://code.djangoproject.com/ticket/12982#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: Alex | Owner:
| berkerpeksag
Component: Core (Cache system) | Version: master
Keywords: cache | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"34fb909180e9df06fa6a993dd5696a49cd152a0b" 34fb9091]:
{{{
#!CommitTicketReference repository=""
revision="34fb909180e9df06fa6a993dd5696a49cd152a0b"
Fixed #12982 -- Added a get_or_set() method to the BaseCache backend.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/12982#comment:21>