Code review comment for lp:~vila/udd/analyze-log-imports

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 09/05/2011 10:08 AM, Vincent Ladeuil wrote:
> > Vincent Ladeuil has proposed merging lp:~vila/udd/analyze-log-imports into
> lp:udd with lp:~vila/udd/analyze_log as a prerequisite.
> >
> > Requested reviews:
> > Ubuntu Distributed Development Developers (udd)
> >
> > For more details, see:
> > https://code.launchpad.net/~vila/udd/analyze-log-imports/+merge/74057
> >
> > Based on https://code.launchpad.net/~vila/udd/analyze_log/+merge/74056, this
> patch parses and reports the successful imports found in the log file.
> >
> > This diverges a slight bit from the initial purpose of the
> > analyze_log.py purpose. If more divergences appear, it may be
> > nice to switch to a more general approach
> > s/report_times(/report('times', or report('imports', / and allow
> > some command-line or config parameter to decide which parts
> > should reported but I don't it need for now.
> >
> >
>
> I'm not sure I understand what you mean by "diverges from the initial
> purpose". Could you clarify a bit?

The initial purpose was to report import times (report_*times*), it now also reports imports and imports != times.

>
> Would you want to add some test case to the matching logic? (They should
> be fairly easy to write as unit tests.)

That's one exception to TDD to me: I don't think automated tests add value here because I have real data to play with, both from the past and more importantly, live data from the running importer. So it's already easy enough to catch errors and fix them.

Regressions doesn't really matter either as we don't want to maintain compatibility with old logs anyway, if these logs needs to be analysed, it's far easier to just retrieve the old version of analyze_log.py.

>
> + # Cough, not really times... cough
>
> ^- what are they then? The comment doesn't really add any clarity.

48 + # Cough, not really times... cough
49 + for distro, release, version in self.versions:
50 + out.write('Imported: %-10s %-12s %s\n' % (distro, release, version))

It displays the distro, the release and the version that was imported, those aren't times.