Code review comment for lp:~fginther/ubuntu-ci-services-itself/lander-binary-fixes

Revision history for this message
Andy Doan (doanac) wrote :

46 + packages = request_parameters.get('added_binaries', '')
47 + if packages:
48 + packages = packages.split(',')

I don't think you need 47 & 48, python's split should be safe there.

63 + parameters = {'added_binaries': ','.join(packages),
64 + 'removed_binaries': None}

Bikeshedding, but most of our code is declaring dictionaries styled like:

  parameters = {
      'added_binaries': ','.join(packages),
      'removed_binaries': None
  }

20 + if added_list:
21 + for package_name in added_list.split(','):
22 + if package_name not in package_list:
23 + # Only add packages not already in the global list
24 + package_list.append(package_name)
25 +
26 + # Finally remove the list of removed binaries from the ticket request
27 + removed_list = request_parameters.get('removed_binaries', '')
28 + if removed_list:
29 + for package_name in removed_list.split(','):
30 + try:
31 + package_list.remove(package_name)
32 + except ValueError:
33 + # This is ok, multiple queued tickets could be trying
34 + # to remove the same binary.
35 + logger.info('Package to remove not found: '
36 + '{}'.format(package_list))
37 +

Not a big deal, and I could see some people advocate what you have there. But I often find it easier to use Python "sets" for things like this. I think you could simplify this to something like:

 packages = set(ticket.get_binaries())
 packages = packages + set(request_parameters.get('added_binaries', '').split(','))
 packages = packages - set(request_parameters.get('removed_binaries', '').split(','))

None of my comments are really a big deal as the code looks like it works to me. I'll just ack the MP and you can decide whether or not you want to do anything with my comments.

review: Approve

« Back to merge proposal