[Linaro-validation] code review: lava android benchmark code

Paul Larson paul.larson at linaro.org
Tue Feb 7 05:47:49 UTC 2012


Zygmunt, have you had a chance to review the changes that Andy pointed at
below?  It's a bit different from the merge proposal process we are
accustomed to, but pretty easy to sort out.  Andy can correct me if I'm
wrong, but I think you can just sign in with your linaro google id, setup
the account, and review the changes.

If there are no blockers, any chance we can roll this into production
tomorrow?  Andy has his demo in the afternoon, so it would be nice if we
can get him setup by then.  He may require staff access at least so that he
can access the admintool.

Thanks,
Paul Larson

On Fri, Feb 3, 2012 at 3:29 PM, Andy Doan <andy.doan at linaro.org> wrote:

> Thanks for the review. You've taught me about some interesting things in
> Django. Comments in-line.
>
> On 02/03/2012 03:24 PM, Zygmunt Krynicki wrote:
> > Hi Andy.
> >
> > I'm reading the code now. I have some comments. Essentially before this
> > lands you _have_ to add one change, then we can follow up with iterative
> > improvements.
> >
> > You _have_ to apply south migrations to your application. Without that
> > we will not be able to make reasonable upgrades. Adding migrations later
> > is not an option as there is a bit that is very hard to automate.
> >
> > Fortunately this is pretty easy:
> >
> > 1) Add a dependency on south (install_requires in setup.py)
> > 2) Run lava-server manage ... android_benchmark_views_app
> > schemamigration --initial --auto
> > 3) Inspect the code briefly and add it to version control
> > 4) Push that up for review.
>
> I have a patch ready for review:
>  http://review.android.git.linaro.org/#change,1463
>
>
> > As for the rest:
> >
> > *) helpers.py, don't use double leading underscore.
>
> http://review.android.git.linaro.org/#change,1464
>
> > *) Try keeping the code that relates to your model in the model class,
> > also add a few one liners if you can to document each public method
>
> I added some comments to models.py to help explain things:
>  http://review.android.git.linaro.org/#change,1468
>
> > *) extension.py, in def version(), pass the second argument to
> > versiontools.format_version() please pass your main package object
> > (android_benchmark_views_app). This will make it follow git version
> hashes.
>
> I'm not positive I did what you were wanting, but:
>
>  http://review.android.git.linaro.org/#change,1465
>
> > *) models.py, BenchmarkRun, each ForeignKey should have a related_name.
> > This name is used when accessing the model field from the "remote" end,
> > then you can navigate back using this name. In general this is required
> > in some cases (when there are clashes, anyway this is beyond the scope
> > of this conversation)
>
> http://review.android.git.linaro.org/#change,1466
>
> > *) models.py BenchmarkReport, don't use comments like that. Firs of all,
> > in django it's very very bad to use nullable text/char fields. Instead
> > always use null=False, blank=True - that makes writing code then saner.
> > As for comments. Instead of using a simple text field please consider
> > using django comment system. It _is_ slightly more complicated than
> > using a free-for-all text area (like on launchpad's whiteboard but at
> > the same time you get much more in return. You can read about this here:
> > https://docs.djangoproject.com/en/dev/ref/contrib/comments/
>
> So for now I've just changed the nullable to be false. I'm not sure the
> django comment system is what we need for this. Basically in this case I
> intend "comments" to be where you can put a bit of an "executive
> summary" for the given report. The change I made added some help_text to
> the field to help convey that.
>
>  http://review.android.git.linaro.org/#change,1470
>
> Let me know if you think this is adequate or if the comment system still
> would be better. It looks easy enough to hook up.
>
> > *) models.py both classes, add a get_absolute_url() method based on
> > django permalink system. It's trivial to use, see this document:
> >
> https://docs.djangoproject.com/en/dev/ref/models/instances/#django.db.models.permalink
>
> http://review.android.git.linaro.org/#change,1467
>
> > *) You ship query.flot. While that's okay for now I'd rather see this
> > managed by a dedicated django app that just wraps this in one place. As
> > for now this can stay as-is.
>
> Yeah, that probably makes sense. My versions have some handy fixes that
> took me a while to come up with, but would be useful to others wanting
> to do some charting.
>
> We (read I) should probably look into submitting patches for this, but I
> see so many flot patches floating around that I assume the maintainers
> probably aren't interested.
>
> > *) Your templates extend dashboard templates. Please don't do that (as
> > those templates are not a stable/public interface). Instead extend
> > lava-server templates that work just as well but are stable.
>
> http://review.android.git.linaro.org/#change,1469
>
> >
> > That's a very quick review. I have one more comment.
> >
> > EXCELLENT WORK :-)
>
> I cheated and tried to copy all the great work your team has done.
>
> _______________________________________________
> linaro-validation mailing list
> linaro-validation at lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-validation
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linaro.org/pipermail/linaro-validation/attachments/20120206/4c8a765a/attachment.html>


More information about the linaro-validation mailing list