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