Code review comment for lp:~malept/pyexiv2/0.2-ReadMetadataTestCase

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

> > I have one remark on the consistency of the checks: the original case tests
> > the python type of the values of EXIF and IPTC tags. In the case you added,
> > the check is on the tag's type, not the type of the corresponding python
> > object.
>
> So you want to check tag.type (e.g., 'Ascii' for some strings)? I presume that
> you don't want to check tag.__class__ anymore, because that seems to always be
> ExifTag.

No, for the sake of consistency, I would rather check tag.value.__class__, as is done for EXIF and IPTC tags.

> > Also, how about merging some selected XMP metadata from exiv2-bug540.jpg
> into
> > smiley1.jpg using exiv2 itself, rather than doing the testing on two
> different
> > files, and what's more on a very large number of XMP tags?
>
> I can do that. I only did it this current way because it was easy to extract
> all of the XMP tags and throw them into the testcase. I presume I should put
> one (or two) XMP tags per type into smiley1.jpg?

No need to do it in a first step, the patch is correct without. Let's first merge the patch and then enhance the test case in a second step.

« Back to merge proposal