Ubuntu Distributed Development

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

analyze-log-imports
Merge into import-scripts

Revision history for this message

John A Meinel (jameinel) wrote on 2011-09-06:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/06/2011 10:50 AM, Vincent Ladeuil wrote:
>> -----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.
>

You don't know if you are missing things running against the live
importer. (AFAIK.)

> 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.
>

The comment still doesn't add value, IMO.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk5l8AwACgkQJdeBCYSNAANAFgCdGZUPNy6LWYtuNyAhNHUg71SK
8OgAoNWSCGS6mZXt88nkJ7LcRqszJElU
=KGAm
-----END PGP SIGNATURE-----

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/06/2011 10:50 AM, Vincent Ladeuil wrote:
>> -----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.
>

You don't know if you are missing things running against the live
importer. (AFAIK.)

> 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.
>

The comment still doesn't add value, IMO.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk5l8AwACgkQJdeBCYSNAANAFgCdGZUPNy6LWYtuNyAhNHUg71SK
8OgAoNWSCGS6mZXt88nkJ7LcRqszJElU
=KGAm
-----END PGP SIGNATURE-----

« Back to merge proposal