Code review comment for lp://qastaging/~cmiller/desktopcouch/dbus-refs-explicit-and-rm-resolve-on-disappear

Revision history for this message
Eric Casteleijn (thisfred) wrote :

in replication.py, the following imports are unused:

import threading
import logging.handlers
import desktopcouch

this is unconventionally indented and uses an undefined variable 'sn' (occurs twice):

        log.error("The service %r is unknown. It is not a "
                "module in the %s package ." % (sn, container))
        return ""

this has an undefined variable 'remote_identifier'

                        couchdb_io.expunge_pairing(remote_identifier)

and there are several long lines.

dbus_io

args is undefined in:

             logging.error("annc doesn't look like one of ours. %r", args)

also, this scares me:

class LocationAdvertisement(Advertisement):
    """An advertised couchdb location. See Advertisement class."""
    def __init__(self, *args, **kwargs):
        if "stype" in kwargs:
            kwargs.pop(stype)
        super(LocationAdvertisement, self).__init__(
                stype=location_discovery_service_type, *args, **kwargs)

class PairAdvertisement(Advertisement):
    """An advertised couchdb pairing opportunity. See Advertisement class."""
    def __init__(self, *args, **kwargs):
        if "stype" in kwargs:
            kwargs.pop(stype)
        super(PairAdvertisement, self).__init__(
                stype=invitations_discovery_service_type, *args, **kwargs)

Isn't it possible to make it a little more explicit? I think I would prefer:

kwargs.pop(kwargs["stype"])

review: Needs Fixing

« Back to merge proposal