launchpad-reviewers team mailing list archive

[Benji York <benji.york@xxxxxxxxxxxxx>]

Review: Approve code

This branch looks good.  Here are some minor comments:

It seems you have an extra underscore in the test name
"test_create__bad_artifact_class".

I like the generalization of _assert_artifact_change_unsubscribes.  It is
unfortunate that the function is so long though.  I wonder if instead of
passing in a test type we could instead break out the contents of that if block
into functions that are instead passed in and called.

Another idea: break the _assert_artifact_change_unsubscribes into two
functions, one for the code before "if test_type" and one after.  The
individual assertions could then call the first function, then include their
unique code (currently contained in the if) and then finally call the last
function.

-- 
https://code.launchpad.net/~adeuring/launchpad/sharingjob-remove-bp-subscriptions/+merge/126008
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.