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

> 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.

added_binaries and removed_binaries can be defined as None in which case "AttributeError: 'NoneType' object has no attribute 'split'" can be raised.

> 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
> }

I wasn't aware of this, I can switch.

> 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(','))

I like your recommendation here, it's much cleaner. Unit tests are in place, so it's an easy change to test.

> 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.