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
=== modified file 'src/pyexiv2/metadata.py'
--- src/pyexiv2/metadata.py 2010-09-29 21:12:19 +0000
+++ src/pyexiv2/metadata.py 2010-09-30 08:33:40 +0000
@@ -31,6 +31,8 @@
31import os31import os
32import sys32import sys
33from errno import ENOENT33from errno import ENOENT
34from collections import MutableMapping
35from itertools import chain
3436
35import libexiv2python37import libexiv2python
3638
@@ -40,7 +42,7 @@
40from pyexiv2.preview import Preview42from pyexiv2.preview import Preview
4143
4244
43class ImageMetadata(object):45class ImageMetadata(MutableMapping):
4446
45 """47 """
46 A container for all the metadata embedded in an image.48 A container for all the metadata embedded in an image.
@@ -321,6 +323,12 @@
321 except AttributeError:323 except AttributeError:
322 raise KeyError(key)324 raise KeyError(key)
323325
326 def __iter__(self):
327 return chain(self.exif_keys, self.iptc_keys, self.xmp_keys)
328
329 def __len__(self):
330 return len( [ x for x in self ] )
331
324 def _get_comment(self):332 def _get_comment(self):
325 return self._image._getComment()333 return self._image._getComment()
326334
327335
=== modified file 'test/metadata.py'
--- test/metadata.py 2010-09-29 21:12:19 +0000
+++ test/metadata.py 2010-09-30 08:33:40 +0000
@@ -601,3 +601,49 @@
601601
602 self.failUnlessEqual(self.metadata.comment, self.other.comment)602 self.failUnlessEqual(self.metadata.comment, self.other.comment)
603603
604 #############################
605 # Test MutableMapping methods
606 #############################
607
608 def _set_up_clean(self):
609 self.clean = ImageMetadata.from_buffer(EMPTY_JPG_DATA)
610
611 def test_mutablemapping(self):
612 self._set_up_clean()
613 self.clean.read()
614
615 self.assertEqual(len(self.clean), 0)
616 self.assertTrue('Exif.Image.DateTimeOriginal' not in self.clean)
617
618 key = 'Exif.Image.DateTimeOriginal'
619 correctDate = datetime.datetime(2007,03,11)
620 incorrectDate = datetime.datetime(2009,03,25)
621 tag_date = ExifTag(key,correctDate)
622 false_tag_date = ExifTag(key,incorrectDate)
623 self.clean[key] = tag_date
624
625 self.assertEqual(len(self.clean), 1)
626 self.assertTrue('Exif.Image.DateTimeOriginal' in self.clean)
627 self.assertEqual(self.clean.get('Exif.Image.DateTimeOriginal', false_tag_date), tag_date)
628 self.assertEqual(self.clean.get('Exif.Image.DateTime', tag_date), tag_date)
629
630 key = 'Exif.Photo.UserComment'
631 tag = ExifTag(key,'UserComment')
632 self.clean[key] = tag
633 key = 'Iptc.Application2.Caption'
634 tag = IptcTag(key,['Caption'])
635 self.clean[key] = tag
636 key = 'Xmp.dc.subject'
637 tag = XmpTag(key, ['subject', 'values'])
638 self.clean[key] = tag
639
640 self.assertTrue('Exif.Photo.UserComment' in self.clean)
641 self.assertTrue('Iptc.Application2.Caption' in self.clean)
642 self.assertTrue('Xmp.dc.subject' in self.clean)
643
644 self.clean.clear()
645 self.assertEqual(len(self.clean), 0)
646
647 self.assertTrue('Exif.Photo.UserComment' not in self.clean)
648 self.assertTrue('Iptc.Application2.Caption' not in self.clean)
649 self.assertTrue('Xmp.dc.subject' not in self.clean)

Subscribers

People subscribed via source and target branches