harvest-dev team mailing list archive

[Daniel Holbach <daniel.holbach@xxxxxxxxxx>]

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


-- 
https://code.launchpad.net/~dylanmccall/harvest/harvest-dylan-m/+merge/28262
Your team harvest-dev is requested to review the proposed merge of lp:~dylanmccall/harvest/harvest-dylan-m into lp:harvest.