Code review comment for lp:~dylanmccall/harvest/harvest-dylan-m

> Oh, I forgot. This also adds a dependency on debug-toolbar. That change can be
> reverted (it's just in settings.py and INSTALL), but it also doesn't hurt.
> It's very useful for tracking how changes affect performance.
>
> Note that the middleware adds a whole ton of data to the page as it is sent
> (when enabled), so it does have a performance hit of its own. It only turns on
> if it's being accessed through 127.0.0.1.

Maybe we can ask people to add that stuff to local_settings.py or we require YES_I_AM_HARVEST_HACKER to be True? We could also test if debug_toolbar can be imported.

About the rest of the new code: HOLY COW! You put quite a bit of work into this! :)

FilterContainer:
~~~~~~~~~~~~~~~~
minor only, but the description of set_filters() says "Add a set of filters to be children of this one." although self.filters_dict is reset at the beginning. Would an add_*() function be useful or should the description just be modified a bit?

I think I'd rename get() to find(), not sure which is more common. Both work for me. :-)

113 + #the first bit is the one this instance stores
114 + if name_bits[0] in self.filters_dict:
115 + result = self.filters_dict[name_bits[0]]
116 + else:
117 + result = None

Lines 116 and 117 can be removed.

119 + if isinstance(result, FilterContainer) and len(name_bits)>1:
120 + return result.get(name_bits[1]) #send remaining bits to child
121 + else:
122 + #we have reached the end of the list. Return the result (which may be None)
123 + return result

121 can be removed and 123 unidented one level.

FilterSystem:
~~~~~~~~~~~~~

Does every call to get_parameters() produce a copy? Do we need a get()ter there?

Filter:
~~~~~~~

Do we need explicit getters and setters() for the very simple members of Filter?

Do you think it's useful to add a few very quick examples to some of descriptions? ie: to me it wasn't immediately obvious what the "toggling" is about.