launchpad-reviewers team mailing list archive

[Brad Crittenden <bac@xxxxxxxxxxxxx>]

Review: Approve code

Hi James,

Thanks for making this change.  I hadn't paid a lot of attention to the new work items feature of blueprints so it was interesting to learn a little about them.

* The logic to populate a dictionary with the count of items in a list is pretty generic and repeated.  Perhaps you could factor that out into a method.

* Please change "if(" to "if (" at line 17 and elsewhere.

* I think "if not title in existing_title_count" reads better as "if title not in existing_title_count", as you'd written it previously.

* In the following

   if(new_wi['title'] not in existing_titles or
      new_wi['title'] in existing_title_count and
      existing_title_count[new_wi['title']] == 0):

the second line is redundant as you just established the title is in existing_titles so it must also appear in existing_title_count.

* I know you didn't write this line, but

for i in range(len(new_work_items)):
    new_wi = new_work_items[i]

would be easier to follow if it was done like

for i, new_wi in enumerate(new_work_items):

* You have a typo "anotehr".

* In your test, it might be easier to understand if your duplicate items were copied from the first one, i.e.

from copy import copy
new_w1_data = dict(...)
new_w2_data = copy(new_w1_data)
...
new_w3_data = copy(new_w1_data)

A reader will know instantly what is going on without having to look at every item in the dicts to verify they are the same.

Finally I'll note this test is very busy.  Ideally each test would exercise one concept.  Perhaps you could break this test up into one that shows adding duplicates works and another that proves deleting will work.  That separation comes at the price of refactoring or (worse) duplicate set up code.

I'd like to go ahead and mark this proposal 'Approved', pending the mentioned changes being made.

Thanks for the nice branch.

-- 
https://code.launchpad.net/~dooferlad/launchpad/workitems-bugfix/+merge/110563
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.