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

Revision history for this message
Kevin McDermott (bigkevmcd) wrote :

=== 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?

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

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?

And running PEP8 on view.py is horrific...but hey :-)

review: Needs Fixing

« Back to merge proposal