Code review comment for lp:~linaro-infrastructure/launchpad/workitems-parsing-and-updating

On 24/02/12 12:43, Abel Deuring wrote:
> Review: Approve code
>
> Hi Salgado,
>
> the changes look good overall, I just have a few formal nitpicks, see below.

Thanks, Abel!

>
>> + def updateWorkItems(new_work_items):
>> + """Update the existing work items to match the given ones.
>> +
>> + First, for every existing work item that is not present on the new
>> + list, mark it as deleted. Then, for every tuple in the given list,
>
> s/tuple/dictionary/

Oops, fixed.

>
>> + lookup an existing work item with the same title and update its
>> + status, assignee, milestone and sequence (position on the work-items
>> + list). If there's no existing work items with that title, we create a
>> + new one.
>> +
>> + :param new_work_items: A list of dictionaries containing the following
>> + keys: title, status, assignee and milestone.
>> + """
>
>> + def _deleteWorkItemsNotMatching(self, titles):
>> + """Delete all work items whose title does not match the given ones.
>> +
>> + Also set the sequence of those deleted work items to -1.
>> + """
>
> Since the work item will not literally be deleted, I think "Mark all
> work items as deleted" is a bit more precise than "Delete all work items"

Good point, changed.

>
>
>> + for work_item in self.work_items:
>> + if work_item.title not in titles:
>> + work_item.deleted = True
>> +
>> + def updateWorkItems(self, new_work_items):
>> + """See ISpecification."""
>> + # First mark work items with titles that are no longer present as
>> + # deleted.
>> + self._deleteWorkItemsNotMatching(
>> + [wi['title'] for wi in new_work_items])
>> + work_items = Store.of(self).find(
>> + SpecificationWorkItem, specification=self, deleted=False)
>> + work_items = list(work_items.order_by("sequence"))
>> + # At this point the list of new_work_items is necessarily the same
>> + # size (or longer) than the list of existing ones, so we can just
>> + # iterate over it updating the existing items and creating any new
>> + # ones.
>> + to_insert = []
>> + existing_titles = [wi.title for wi in work_items]
>> + for i in range(len(new_work_items)):
>
> Our style guide does not allow single-letter variable names. WHat about
> "for position in..." or "for index in..."?

I don't think this should apply to a counter variable used in a for
loop, but I'm happy to rename it to index. :)