Re: ITM: Please review lab-refactor branch
On 2011-11-01 07:04, Adam D. Barratt wrote:
> On Thu, 2011-10-27 at 21:52 +0200, Niels Thykier wrote:
>> The major part of the development in the lab-refactor branch should be
>> done now, so I intend to merge it into master. The branch has touched
>> major parts of Lintian and breaks a lot of stuff. Obviously, I would
>> like a thorough review of it before I hit merge - particuarly of
>> reporting/* as we have no test-suite coverage here.
>>
>> I would like to see the merge done some time next week followed by an
>> upload. However if you would like more time to review the branch,
>> please let me know how long you need.
>
> I started having a look over the branch at the weekend; unfortunately a
> combination of being busy with other (mostly work-related) stuff and not
> feeling great means it hasn't been as useful as I'd hoped so far.
> Nevertheless I thought it was worth recording my thoughts so far.
>
Thanks for the comments so far. :)
> * As discussed on IRC, it may be worth considering whether we want to
> s/lab_// in a number of method names.
>
Yes, I do not feel strongly either way. As I recall I added it because
some methods had the "lab" marker and others didn't. Btw, I presume you
also want "s/_lab//" (i.e. create_lab => create)?
> * How much of reporting/ has been tested with the new lab code? I
> realise we don't have formal test coverage for it but from memory
> getting a minimal setup working is fairly simple.
>
Not a lot to be honest. I have tested html_reports with the standard
log file from lintian.d.o and an empty lab. Beyond that, "perl -Ilib -c
$script" is about the testing it has received.
> * I noticed there's code intended for v10 lab format backward
> compatibility. Given that the patch also updates the lab format, how
> much has that compatibility code been tested?
>
The only compatibility code is "lab_exists" and "remove_lab", because
else the API cannot remove an existing v10 Lab. I just tested it on my
v10 lab. It fails to open the lab and can remove the lab.
I did consider adding a migration script that could migrate a v10 Lab to
a v11, but I believe it will very limited in usefulness. Particularly,
on the first upload after merging we will probably need a full run to
pick up the packages not checked right now.
> * Finally whilst having an initial run through the code I made a number
> of tweaks and reminder comments, mostly related to documentation. I'm
> afraid it's not nicely split up but I've attached a copy of the changes
> in the hope that they're useful.
>
Thank you. :)
> Regards,
>
> Adam
~Niels
Reply to: