Code review comment for lp:~newell-jensen/maas/fix-1665839

Revision history for this message
Christian Reis (kiko) wrote :

On Tue, Feb 21, 2017 at 07:29:29PM -0000, Newell Jensen wrote:
> - discovered_machine = [m for m in new_machines if m not in machines][0]
> + discovered_machine = [
> + nm for nm in new_machines if (
> + nm.power_parameters.get('node_id') not in [
> + m.power_parameters.get('node_id') for m in machines])][0]

Ugh, I really don't like these nested comprehensions. The code is super
hard to understand!

Also, I don't like the assumption that this will only have one useful
item, which is not very defensive. Is this necessarily always
new_machines?

How about:

    discovered_machines = []
    power_node_ids = [m.power_parameters.get("node_id") for m in machines]
    for nm in new_machines:
        if nm.power_parameters.get('node_id') not in power_node_ids:
            discovered_machines.append(nm)

    # if you actually can guarantee that you are only getting one
    assert len(discovered_machines) == 1
    discovered_machine = discovered_machines[0]

This code may need a deeper look, as it's not very intuitive, and I
don't know it well.

« Back to merge proposal