Code review comment for lp:~mterry/ubuntu-archive-tools/or-deps

Revision history for this message
Colin Watson (cjwatson) wrote :

To be honest, I'm a bit sceptical about doing this at all. In many
cases, even if a dependency technically can be satisfied using one of
the other elements in a disjunction, we really should be updating the
outdated elements anyway; consider the recent case of dhcp3-client,
where it was entirely correct to update at least some of those elements
to say isc-dhcp-client instead. I'm concerned that, if these stop
showing up in nbs-report, we'll stop bothering; and there's really
nowhere else we automatically notice this kind of thing.

How about, instead of removing these packages from the NBS report
altogether, we just put a note against them? Something like "but
alternatives are present: C, D" would provide us an immediate scannable
visual clue that a disjunction is involved, without losing information
or risking false positives.

A few minor code nits:

> + yield (dep, dep_list)
[...]
> + for (depname, depwhy) in parse_relation_packages(stanza[field]):
[...]
> + for (depname, depwhy) in parse_relation_packages(stanza[field]):

These days I tend to prefer to drop the unnecessary parentheses in these
cases.

> + # Check why this rdep is here. If it is part of an or-group, maybe
> + # one of the other rdeps in that group are available, and this rdep
> + # should not be blocking removal of pkg.
> + ignore = False
> + why = nbs[pkg][rdep][1]
> + for whydep in why.split('|'):
> + if whydep not in nbs:
> + ignore = True
> + break
> + if not ignore:
> + try:
> + checked_v.remove(rdep)
> + except KeyError:
> + pass
> + return False

This may be obsolete given my comments above, but FWIW, this could be
more concisely written using Python's handy for/else construction:

    why = nbs[pkg][rdep][1]
    for whydep in why.split('|'):
        if whydep not in nbs:
            break
    else:
        try:
            checked_v.remove(rdep)
        except KeyError:
            pass
        return False

« Back to merge proposal