On 10 Mar 10:04, Dan McGee wrote:
Nice! I'd be glad to have this hosted on the main site (I'm the maintainer). Overall things look sound; sure there are some style/convention/etc. differences but nothing hard to fix up.
* View page: right now it is making 98 queries (one for each of those max values). Not that this page is probably going to see loads of traffic but we can surely improve upon that. I would prefer to change that, yes, I was having a lot of trouble figuring out a nice way to get this kind of structure in the page. So if you or anyone else can point me in another direction it would be much appreciated.
* Add page: Using radio buttons for widgets here would probably be smarter than the dropdowns, otherwise field entry is excruciatingly painful. See the forms documentation for how to change the default widget. I'll look at it, that does sound like it could be nicer.
* models, Test.ms: totally un-descriptive field name, and you didn't give it a nice field name. Can't this just be "modules"? Yes I'll do that.
* models, Test: if you name things boot_type instead of boottype, you'll get nicer default labels on your view and entry screens. Same with the models themselves- Boottype -> BootType. Ok I'll do that.
* models, Iso: Likely need more than a date field here. We've double released, etc. It might just be worth having a CharField as no users are going to be entering these anyway, and also having a boolean "active" field or something to determine what actually shows up on the entry and view pages. Sure, I didn't know what to put in it (or most of the models for that matter) anyway.
* models, get_success_test/get_failed_test: these can be abstracted into a common proxy model (http://docs.djangoproject.com/en/1.2/topics/db/models/#proxy-models) superclass, from which all these various ISO options inherit from. Ah ok, I was thinking of using a base model for that, but in the interest of being done as soon as possible I used this way first. I'll look at this.
* tests.py: yes, we should be writing tests, but at the least, get rid of the default garbage in this file. Oh heh, I didn't write that file, I'll remove it.
* admin.py: convention is not to use * imports; I know it works here and makes things cleaner but I've eradicated the codebase of them otherwise. Ok I'll get rid of all of them.
* models.py: "# Create your models here." -> not needed You're right.
* urls.py: You aren't using info_dict anymore; you have a blank second pattern definition?, and I'd prefer you follow indentation patterns of something like packages/urls.py. Right again, I'll look at the indentation, I was thinking about that anyway, it's not nice to not adhere to someone else's style rules, I just sent first and wanted to clean up later.
* views.py, add: A lot of extra comments. "Create your views here.", all the "form been submitted" trailing stuff. For the return, I've switched to using mostly direct_to_template to avoid some of the boilerplate- see the end of flag() in packages/views.py around line 373 for an example. I'll look at that too.
* views.py:, view: I see some things are _choices, some are _list- why the disparity? Hehe, that's because in the beginning I thought it might be better to use the choices parameters for some things, but then later I switched and I apparently didn't want to rename the fields then, I think I was in a rush at the time, sorry.
* isotest/templates: why is this in a different directory from the standard layout? Oh I'm sorry, that should have already been removed. That's there because I read in the django documentation that templates _should_ also be looked for in an app's directory, I thought it'd be nicer if I completely seperated my app from the rest of archweb so it could have been placed in any project if it wasn't going to be placed into archweb. But I couldn't get that to work right away so moved on again.
* Finally, I see 4 + 1 templates, but only two views- are these other ones old, unnecessary, not wired up yet? They must be from tests with object_list generic functions and such.
Great work! I know I just said a lot but these are mostly minor things, nothing to feel bad about. Thank you! I appreciate your comments, I will get to work on everything you've mentioned.
Tom