Code review comment for lp:~allenap/launchpad/dynamic-batch-size-bug-546085

Hi Gavin,

This is a nice branch. I just have two comments besides the lint errors.

merge-conditional

-Edwin

== Pyflakes notices ==

lib/lp/bugs/externalbugtracker/debbugs.py
    124: local variable 'severity' is assigned to but never used

lib/lp/bugs/scripts/checkwatches/tests/test_updater.py
    157: local variable 'bug' is assigned to but never used

>=== modified file 'lib/lp/bugs/externalbugtracker/debbugs.py'
>--- lib/lp/bugs/externalbugtracker/debbugs.py 2010-03-16 16:52:42 +0000
>+++ lib/lp/bugs/externalbugtracker/debbugs.py 2010-03-31 15:33:31 +0000
>@@ -61,7 +61,7 @@
> # Because we keep a local copy of debbugs, we remove the batch_size
> # limit so that all debbugs watches that need checking will be
> # checked each time checkwatches runs.
>- batch_size = None
>+ batch_size = 0

Zero is a magic number. It would be good to define it as a constant
as opposed to relying on comments to explain what it means. You might
want to define the constant as a unique object such as:
    UNLIMITED = object()
so that there is never the possiblity of someone using zero and getting
the opposite results than they expect. That would allow you to get
rid of this if-statement in _getRemoteIdsToCheck() where you change
the meaning of the magic values.
    if batch_size == 0:
        batch_size = None

>
> def __init__(self, baseurl, db_location=None):
> super(DebBugs, self).__init__(baseurl)
>
>=== modified file 'lib/lp/bugs/scripts/checkwatches/updater.py'
>--- lib/lp/bugs/scripts/checkwatches/updater.py 2010-03-26 21:03:30 +0000
>+++ lib/lp/bugs/scripts/checkwatches/updater.py 2010-03-31 15:33:31 +0000
>@@ -242,6 +247,22 @@
> yield item
>
>
>+def suggest_batch_size(remote_system, num_watches):
>+ """Suggest a value for batch_size if it's not set.
>+
>+ Givend the number of bug watches for an `remote_system`, this sets
>+ a suggested batch size on it. If `remote_system` already has a
>+ batch size set, this does not override it.
>+
>+ :param remote_system: An `ExternalBugTracker`.
>+ :param num_watches: The number of watches for `remote_system`.
>+ """
>+ if remote_system.batch_size is None:
>+ remote_system.batch_size = max(
>+ SUGGESTED_BATCH_SIZE_MIN, int(
>+ SUGGESTED_BATCH_SIZE_PROPORTION * num_watches))

I think it would be easier to read with this wrapping.
        remote_system.batch_size = max(
            SUGGESTED_BATCH_SIZE_MIN,
            int(SUGGESTED_BATCH_SIZE_PROPORTION * num_watches))

>+
> class BugWatchUpdater(object):
> """Takes responsibility for updating remote bug watches."""
>