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

Yay! Thanks for the comments. I'll respond to them here.

There is a bit of superfluous initialization (by Python standards) going
on, indeed. Thanks for pointing them out! I was still in Vala mode when
I started this, desperately clinging to the comfort of strong typing ;)

So, in order from the top!

---FilterContainer---

        FilterContainer is currently used by two classes: FilterGroup
        under filters/filters.py, and FilterSystem under
        filters/containers.py. It is a bit wacky at the moment.
        FilterSystem is the one single root container that all the
        filters go inside of. It should be The object that the rest of
        the application uses to manipulate filters. The current HTTP
        request is handed to that object and it sorts out the rest.
        Thankfully, that means the thing can be poked at repeatedly
        until the wackiness subsides, without affecting the rest of the
        code.
        FilterGroup is used to group filters that work with a specific
        collection of objects (there's a pkg and opp group at the
        moment). It is possible to enable and disable filters inside a
        FilterGroup.

        set_filters() probably made sense the way it was at some point
        in the past. It will still only ever be called once, but I
        needn't enforce that; it just adds complexity for no particular
        reason. I renamed set_filters to add_filters and I'm
        initializing self.filters_dict under __init__. All these
        functions starting with set_ and get_ were making my head hurt,
        anyway.

        get_parameters(), as it turns out, was just redundant and isn't
        used anywhere any more. I'll strip that out. Similar
        functionality happens behind the scenes in url_tools.py, with
        current_url_with_parameters(). Come to think of it, there's _a
        lot_ of copying going on for one request. It can probably be
        sped up somewhere.

        FilterContainer.get() does imply a similarity to the get()
        method of many other types, even though it is definitely not
        like those. Good point. Changed it to find()!

        Daniel, you're awesome at naming functions! :)

        Thanks for spotting the leftover set_parameters stuff, James.
        It's all gone now! (*phew*)

---Filters---

        The default parameters are just for testing purposes, and indeed
        make no sense. I'm using gedit as a consistent query to test
        against, so when it looks exactly like the mockup I'll know! (It
        also needs some kind of weird default, because it doesn't handle
        big lists of packages very gracefully).
        pkg:name is set as such because there is no interface to change
        it yet, except editing the URL. Normally, of course, it would be
        blank.
        (Unfortunately, said interface won't be pretty. Solving it with
        Javascript is easy, but doing it without needs a pretty
        convoluted HTML form. I may just do the Javascript solution for
        now).

        get_value(), get_value_string() and get_parameters_for_value()…
        Okay, I went and played with this. First of all,
        get_value_string had an optional value argument. When blank, it
        would use the object's own value. get_value_string_for, then, is
        the function to override. I scrapped that since it's pointless,
        slightly slower, and confusing. Now that I'm sliding back into
        Python mode, I remember implicit things are bad anyway.

        After that, I went and renamed a bunch of stuff to make sense!

        get_value gets the value as stored in the object, in the format
        native to that object. (Probably a list of strings or a string).
        serialize_value is the inverse of set_value. So, that's the
        string you want to put in the URL's query string to assign the
        value to the object in the future.

        Finally, get_parameters_for_value() returns key / value pairs
        that can be passed to one of the functions in url_tools.py. So
        the key is the full name of the object, the value is the output
        of serialize_value. That function is really out of place in
        Filter, but it is handy… There is probably a much more elegant
        way :)
        The specific function is a bit redundant, so I may just yank it
        out.

        The setters and getters found themselves in such quantity
        because the implementation of these things can fluctuate. A lot
        of that fluctuation has been reduced with the version you're
        seeing here, though. The other reason is I find it elegant to
        have the outside world only access an object through methods;
        never through properties. (That way the rules for accessing a
        given property are self-documenting). If that's silly, let me
        know!

---views.py and wrappers.py---

        The reason I have visible_packages and hidden_packages in
        wrappers.py is that there will be a bunch of other stuff there
        in the near future. For example, stuff that summarizes the two
        collections of packages. Some of this demands processing that
        may or may not happen, so it makes sense to call the appropriate
        functions from the template as appropriate (I think…).

        PackageWrapper is similar; it has little at the moment, but in
        the future could be used to access what categories of
        opportunities lie within, including hidden ones. Throwing that
        logic in the template (even with template tags) feels like a
        horrible act, and probably wouldn't be as efficient.
        Having said that, PackageWrapper feels a bit more wrong because
        there is already a perfectly good SourcePackage object we can
        add data to. I just feel squeamish throwing that stuff directly
        at a SourcePackage model instance. I know it won't save it to
        the database or anything, but it feels wrong somehow.

        I kept an eye on performance when I put that together. It does
        grab the entire list of source packages from the database and
        turn them into new Python objects, but in the end we're just
        doing the same database hit that would happen later. Debug
        Toolbar says we hit the database once for each model; nothing
        blatantly redundant is happening. Always room for better
        performance, though!

---url_tools---

        James, you asked about removing parameters with
        current_url_with_parameters :)

        That can be added if it's necessary. Right now one could
        manipulate request.GET manually and use new_url_with_parameters.
        The URL format I'm using right now means that parameters don't
        really need to be stripped.

        The original function at
        http://djangosnippets.org/snippets/1627/ actually did remove
        parameters if they didn't have values (just occurred to me that
        the behaviour made sense!), but I removed it because it is legal
        to have a parameter that is just a key and no value. (At least,
        I think it is…). At any rate, we need to be able to say “this
        key has been explicitly set to an empty string.”

Thanks for telling me about NotImplementedError, James! Come to think of
it, there are a few functions I've marked as abstract that aren't really
abstract. I'll clean that up and use the exception as appropriate.

Doing cool stuff with Javascript is coming up! It will need some
interesting trickery (modifying all those URLs for filters to stay
current, duplicating some logic in Javascript that already happens in
Python while resisting the temptation to follow the “don't repeat
yourself” philosophy), but it should make the whole thing feel very
responsive. Expanding / shrinking packages in the results will be the
first thing, since that's relatively simple (yet quite effective). It
will require splitting up some templates and view functions, and
figuring out YUI :)

Okay, that's a lot of writing, but this has been really helpful to get
my thoughts straightened. It feels a lot smoother than it did this
morning!

Thanks,
Dylan