Code review comment for lp:~gary/launchpad/bug164196-2

Revision history for this message
Данило Шеган (danilo) wrote :

Conclusions from IRC.

To increase trust in what's going on in parsing whatchanged, we should:
 1) add docstring and unittest for get_key
 2) move duplicateof code to target computed attribute code, and add test
 3) change get_key to use target/attribute

As for the rest, I really like the test coverage: great job there!

And a side note for s/person_causing_change/actor/: wordy name was introduced instead of 'person' to help make code easier to understand/read. It's up to you to decide which you want to keep as long as you take code readability into account :)

Considering we have agreed on a few changes, I am marking this as 'approved', though I'd be happy to take a look at incremental diff if it's a significant change.

review: Approve

« Back to merge proposal