Code review comment for lp:~lifeless/bzr/apply-inventory-delta

Revision history for this message
Martin Pool (mbp) wrote :

18 + # Accumulate parent references (path and id), to check for parentless
19 + # items or items placed under files/links/tree-references.
20 + parents = set()

Please make the comment consistent with what's actually in the set, ie (path, id).

62 + except errors.BzrError, e:
63 + if 'integrity error' not in str(e):
64 + raise

I'd much rather that were an isinstance check, because people would tend to think of the string as something they can freely change in the UI without affecting the behaviour. Also it would let you make the except clause just catch that one type of error.

135 + # before starting to mutate the inventory, as there isn't a rollback
136 + # facility.
137 + list(_check_delta_unique_ids(_check_delta_unique_new_paths(
138 + _check_delta_unique_old_paths(_check_delta_ids_match_entry(
139 + delta)))))

That's very cute. It seems a bit like they should be consumers rather than pass-throughs, but I'm not sure if that would actually be any clearer, or easier to write in Python.

review: Approve

« Back to merge proposal