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.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
