Code review comment for lp:~jelmer/bzr/merge-dir-prober

Revision history for this message
Aaron Bentley (abentley) wrote :

This seems fine in general, but I believe you're changing the signature of from_lines somewhat. Previously, it accepted an iterable, which could be a collection (i.e. list) or an iterator. Now, it seems a collection is mandatory because the iteration is repeated for every prober, but an iterator would be exhausted by the first prober.

Mandating lists may increase memory use, though of course, a given line could be twelve gigabytes if we were really unlucky. At the very least, you should change the docstring if you change the signature.

Because each prober is handled in sequence, you'll also be consuming the whole list of lines for every probe type, even if there's a header at the top of the file, indicating its kind.

Perhaps it would be better to us a coroutine approach instead. For example:

def get_merge_directive_type(lines):
    for line in lines:
        for prober in probers:
            prober.consume(line)
            if prober.merge_directive_type is not None:
                return prober.merge_directive_type

« Back to merge proposal