Code review comment for lp:~salgado/offspring/builder_details

Hi Kevin,

Thanks for the review!

On 11/01/12 05:59, Kevin McDermott wrote:
> Review: Needs Fixing
>
> === modified file 'lib/offspring/web/queuemanager/tests/test_views.py'
>
> + # Stub Lexbuilder.average_load because it executes some SQL that's not
> + # supported by sqlite3 and we're not interested in testing it here.
>
> Can you file a bug to fix this?
>
> I would have fixed it when I was fixing the metrics code, but I'm not sure I fully understand what it's calculating (and the docstring isn't very helpful).
>
> It looks to be calculating the % of the day that builds take for all builds that finish the same day as they start?

Hmm, this was not introduced on this branch, and it was not clear to me
we want to restrict ourselves to the subset of SQL implemented by sqlite3.

>
> === modified file 'lib/offspring/web/queuemanager/views.py'
>
> +def builders(request):
> + # builder_list is a dictionary where keys are Lexbuilder instances to be
> + # shown and the values represent whether or not the information about the
> + # builder's current job should be shown to the current user.
>
> Why no docstring?
>
> +def builder_details(request, builderName):
> + # Here we don't filter out the builders working on private projects (as we
>
> Again, no docstring, and this doesn't handle unknown builder names very well...

Mostly because, IMHO, there wouldn't be much to say other than what one
can easily infer from the function name, but I've added them regardless
as it certainly won't hurt.

>
> Other than that it looks pretty good...
>
> My usual annoyances with things like...
>
> + projects = projects.select_related('project_group').order_by(
> + "project_group")
>
> Why change between different quote types in the same statement?

This also wasn't introduced here...

Ok, now I understand, this branch depends on list-view
(https://code.launchpad.net/~salgado/offspring/list-views/+merge/84291),
which is still up for review. I'll change this on that branch.