pyexiv2

Merge lp:~antasi/pyexiv2/mutablemapping into lp:pyexiv2/0.2.x

  1. mutablemapping
  2. Merge into pyexiv2-0.2
Proposed by Antti Siira
Status: Merged
Merged at revision: 327
Proposed branch: lp:~antasi/pyexiv2/mutablemapping
Merge into: lp:pyexiv2/0.2.x
Diff against target: 88 lines (+55/-1)
2 files modified
src/pyexiv2/metadata.py (+9/-1)
test/metadata.py (+46/-0)
To merge this branch: bzr merge lp:~antasi/pyexiv2/mutablemapping
Reviewer Review Type Date Requested Status
Olivier Tilloy code functional Approve
Review via email: mp+36770@code.launchpad.net

Description of the change

Make ImageMetadata act like a Python MutableMapping.

To post a comment you must log in.
Revision history for this message
Olivier Tilloy (osomon) wrote :

Thanks for the merge proposal!

I noticed the first revision (326) fixes a separate issue which wasn’t detected by the unit tests. Well spotted! I’m going to tweak this revision a bit and merge it first to keep things atomic, then I’ll review the actual implementation of the MutableMapping interface.

About the bug you fixed:
 - the unit tests didn’t catch this problem, I’ll update them in order to avoid regressions in the future
 - list.remove(x) may raise a ValueError, so the try… except… blocks should be adapted

As for the rest of the review, I’m running out of spare time this week and I’ll be away on holidays next week, but be assured that I’ll review it thoroughly when I get back. Thanks again for the valuable contribution!

lp:~antasi/pyexiv2/mutablemapping updated
326. By Olivier Tilloy

When deleting a tag, remove its key from the cache too.
Many thanks to Antti Siira for spotting the issue and proposing a patch.

Revision history for this message
Olivier Tilloy (osomon) wrote :

I pushed revision 326 to lp:pyexiv2 that fixes the keys not being deleted from the cache when a tag is deleted. Would you mind reverting revision 326 in your branch and push? This will ensure the merge proposal and the diff to review are up-to-date.

Revision history for this message
Olivier Tilloy (osomon) wrote :

A quick go at a functional review while I’m at it:

If I understand correctly the documentation of the collections module (http://docs.python.org/library/collections.html#abcs-abstract-base-classes), a class implementing MutableMapping should implement the following methods:

 - __len__ (from Sized)
 - __iter__ (from Iterable)
 - __contains__ (from Container)
 - __getitem__ (from Mapping)
 - __setitem__
 - __delitem__

Your implementation misses __contains__.

review: Needs Fixing (functional)
Revision history for this message
Antti Siira (antasi) wrote :

> A quick go at a functional review while I’m at it:
> If I understand correctly the documentation of the collections module
> (http://docs.python.org/library/collections.html#abcs-abstract-base-classes),
> ...
> Your implementation misses __contains__.

The __contains__ method comes as a mixin method from Mapping class that MutableMapping inherits, so it is not necessary to implement it.
>>> import pyexiv2
>>> i = pyexiv2.ImageMetadata('/var/tmp/20100703T131613-5fe0998c.jpg')
>>> i.read()
>>> i.__contains__
<bound method ImageMetadata.__contains__ of <pyexiv2.metadata.ImageMetadata object at 0xb7395a6c>>
>>>

It is not stated in the documentation, but I think it uses the implemented __iter__ method to go through the keys and check if the searched item is there.

Revision history for this message
Olivier Tilloy (osomon) wrote :

Right, I overlooked that in the documentation.

lp:~antasi/pyexiv2/mutablemapping updated
327. By Antti Siira

Turn ImageMetadata into MutableMapping

Revision history for this message
Olivier Tilloy (osomon) wrote :

Sorry for the very late review.
It’s all good, thanks a lot for your work.
I merged it into trunk at revision 327.

I will try to update the documentation in a near future to reflect the cool things that implementing the MutableMapping interface offers.

review: Approve (code functional)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/pyexiv2/metadata.py'
2--- src/pyexiv2/metadata.py 2010-09-29 21:12:19 +0000
3+++ src/pyexiv2/metadata.py 2010-09-30 08:33:40 +0000
4@@ -31,6 +31,8 @@
5 import os
6 import sys
7 from errno import ENOENT
8+from collections import MutableMapping
9+from itertools import chain
10
11 import libexiv2python
12
13@@ -40,7 +42,7 @@
14 from pyexiv2.preview import Preview
15
16
17-class ImageMetadata(object):
18+class ImageMetadata(MutableMapping):
19
20 """
21 A container for all the metadata embedded in an image.
22@@ -321,6 +323,12 @@
23 except AttributeError:
24 raise KeyError(key)
25
26+ def __iter__(self):
27+ return chain(self.exif_keys, self.iptc_keys, self.xmp_keys)
28+
29+ def __len__(self):
30+ return len( [ x for x in self ] )
31+
32 def _get_comment(self):
33 return self._image._getComment()
34
35
36=== modified file 'test/metadata.py'
37--- test/metadata.py 2010-09-29 21:12:19 +0000
38+++ test/metadata.py 2010-09-30 08:33:40 +0000
39@@ -601,3 +601,49 @@
40
41 self.failUnlessEqual(self.metadata.comment, self.other.comment)
42
43+ #############################
44+ # Test MutableMapping methods
45+ #############################
46+
47+ def _set_up_clean(self):
48+ self.clean = ImageMetadata.from_buffer(EMPTY_JPG_DATA)
49+
50+ def test_mutablemapping(self):
51+ self._set_up_clean()
52+ self.clean.read()
53+
54+ self.assertEqual(len(self.clean), 0)
55+ self.assertTrue('Exif.Image.DateTimeOriginal' not in self.clean)
56+
57+ key = 'Exif.Image.DateTimeOriginal'
58+ correctDate = datetime.datetime(2007,03,11)
59+ incorrectDate = datetime.datetime(2009,03,25)
60+ tag_date = ExifTag(key,correctDate)
61+ false_tag_date = ExifTag(key,incorrectDate)
62+ self.clean[key] = tag_date
63+
64+ self.assertEqual(len(self.clean), 1)
65+ self.assertTrue('Exif.Image.DateTimeOriginal' in self.clean)
66+ self.assertEqual(self.clean.get('Exif.Image.DateTimeOriginal', false_tag_date), tag_date)
67+ self.assertEqual(self.clean.get('Exif.Image.DateTime', tag_date), tag_date)
68+
69+ key = 'Exif.Photo.UserComment'
70+ tag = ExifTag(key,'UserComment')
71+ self.clean[key] = tag
72+ key = 'Iptc.Application2.Caption'
73+ tag = IptcTag(key,['Caption'])
74+ self.clean[key] = tag
75+ key = 'Xmp.dc.subject'
76+ tag = XmpTag(key, ['subject', 'values'])
77+ self.clean[key] = tag
78+
79+ self.assertTrue('Exif.Photo.UserComment' in self.clean)
80+ self.assertTrue('Iptc.Application2.Caption' in self.clean)
81+ self.assertTrue('Xmp.dc.subject' in self.clean)
82+
83+ self.clean.clear()
84+ self.assertEqual(len(self.clean), 0)
85+
86+ self.assertTrue('Exif.Photo.UserComment' not in self.clean)
87+ self.assertTrue('Iptc.Application2.Caption' not in self.clean)
88+ self.assertTrue('Xmp.dc.subject' not in self.clean)

Subscribers

People subscribed via source and target branches