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@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:
- Add a dependency on south (install_requires in setup.py)
- 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...
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@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-validation