Opened 10 years ago

Closed 10 years ago

#4041 closed (fixed)

cache backends should implement __contains__ so that the in operator may be used

Reported by: Gary Wilson <gary.wilson@…> Owned by: Jacob
Component: Core (Cache system) Version: master
Severity: Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

So instead of

cache.has_key(mykey)

one could do

mykey in cache

Attachments (2)

4041.diff (1.0 KB) - added by Gary Wilson <gary.wilson@…> 10 years ago.
added __contains__ to BaseCache class
4041.2.diff (1.0 KB) - added by Chris Beaven 10 years ago.
optimized version

Download all attachments as: .zip

Change History (10)

Changed 10 years ago by Gary Wilson <gary.wilson@…>

Attachment: 4041.diff added

added __contains__ to BaseCache class

comment:1 Changed 10 years ago by Gary Wilson <gary.wilson@…>

Component: UncategorizedCache system
Has patch: set
Needs documentation: set
Triage Stage: UnreviewedDesign decision needed

comment:2 Changed 10 years ago by Simon G. <dev@…>

Triage Stage: Design decision neededReady for checkin

comment:3 Changed 10 years ago by Gary Wilson <gary.wilson@…>

So in my patch, the __contains__ method in BaseCache simply calls has_key. This is nice since all the cache subclasses automatically pick this up for free, but also means that a

mykey in cache

statement becomes slower than

cache.has_key(mykey)

because of the extra function call. If we care about performance here, we might want to implement __contains__ for each cache class without calling that class's has_key method. Looking at the python standard library, it seems that classes with both __contains__ and has_key methods just put the same code in each instead of having one call the other.

comment:4 Changed 10 years ago by Malcolm Tredinnick

Needs documentation: unset
Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

Not yet ready to checkin, due to Gary's last comment. Checking in an optimisation that slows the code down seems like something we should avoid.

Note sure why this was marked as needing documentation, either, but if you think it needs documentation, Gary, by all means write some.

Changed 10 years ago by Chris Beaven

Attachment: 4041.2.diff added

optimized version

comment:5 Changed 10 years ago by Chris Beaven

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

Unless I'm missing something, can't we just do this? (see patch)

comment:6 in reply to:  4 Changed 10 years ago by Gary Wilson <gary.wilson@…>

Replying to mtredinnick:

Note sure why this was marked as needing documentation, either, but if you think it needs documentation, Gary, by all means write some.

I was just thinking it might be good to be mentioned in the cache documentation (there is no mention of has_key() method currently).

comment:7 in reply to:  5 Changed 10 years ago by Malcolm Tredinnick

Replying to SmileyChris:

Unless I'm missing something, can't we just do this? (see patch)

Yes, it is possible to make __contains__ and has_key be aliases for each other like this. In the early days, when __contains__ was first introduced into the language, it wasn't clear if this was going to be a good identification, but nowadays, it's accepted as correct; to the point that even Python core checkins are using this idiom in places.

What that patch doesn't need is the comment: it's the equivalent of saying "add one to i" in a comment. People know what __contains__ does already.

comment:8 Changed 10 years ago by Malcolm Tredinnick

Resolution: fixed
Status: newclosed

(In [5171]) Fixed #4041 -- Added a contains method to cache backends. Thanks, Gary
Wilson and SmileyChris.

Note: See TracTickets for help on using tickets.
Back to Top