Code review comment for lp:~antasi/pyexiv2/mutablemapping

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!

« Back to merge proposal