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
