Code review comment for lp:~jibel/britney/fix_missing_results

> autopkgtest.py
> ==============
>
> - logging.warning(
> - "Invalid line format: '%s', skipped" % line)
> + print("W: Invalid line format: '%s', skipped" % line)
>
> This indeed should be fixed. However, for consistency this should use
> self.__log(..., type='W') instead of print.
>
Actually __log() is a method of britney and used in britney.py. In autopkgtest.py only print() is used.

>
> + if not trigsrc in self.pkglist[src][ver]['causes']:
> + self.pkglist[src][ver]['causes'][trigsrc] = []
> + self.pkglist[src][ver]['causes'][trigsrc].append((trigver,
> + status))
>
> This is a common pattern in Python which is much more succinct with a dict's
> setdefault() method. I. e.

Fixed.

>
> self.pkglist[src][ver]['causes'].setdefault(trigsrc,
> []).append(
> (trigver, status))
>
> It would be helpful to add a docstring to read() to say what it reads (the
> results file), what its format is, and what the structure of the generated
> self.pkgcauses and self.pkglist are.

Added documentation.

>
> Otherwise the new logic seems fine to me (thanks to JB for additional
> explanations on IRC).
>
> britney.py
> ==========
>
> + adt_label = status
> + if status in ADT_EXCUSES_LABELS:
> + adt_label = ADT_EXCUSES_LABELS[status]
>

Fixed.

> Do we realistically expect a status which isn't in the map? If not (and we
> want to get pointed to that error), I'd suggest to simply use the dict lookup.
> If we want to allow that possibility, I suggest this for simplification:
>
> adt_label = ADT_EXCUSES_LABELS.get(status, status)
>

because adt-britney and britney are 2 separate projects, it is possible, while unlikely, that we add new statuses in adt-britney that are not in britney and make britney crash if the status is unknown.
>
> LGTM otherwise.