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

Revision history for this message
Daniel Holbach (dholbach) wrote :

> That Java course nearly
> corrupted me!

BIG HUGS! I'm sure you'll survive, though! :-)

> There is a bit of divergence in filters.py in the gsoc-client-stuff branch, so
> I'm a little reluctant to make that change in this branch. (Merge conflicts —
> even simple ones — invariably give me headaches). Are you okay if I do it in a
> new branch from gsoc-client-stuff?

Sure. I'm also happy for small branches and small merge proposals to go in as it should be much quicker, easier to review, etc.

> > I was under the impression that we wouldn't have to have all
> > opportunities and source packages in memory if the query just asked
> > for a specific subset. Maybe I'm wrong. I'll go and find out.
>
> I mean to say that all the source packages which have been met by the package
> filtergroup are examined in a similar way. It isn't an extensive thing, but we
> end up accessing the queryset, which gives us a list of all the references it
> has found, both hidden and visible packages. (The opportunities remain a
> queryset until the template asks for them). I should move that to be smarter
> about hidden packages, only storing their count via a query.

As I told you in a separate discussion my main concern was merely the complexity of new classes we add. If we (and particularly new contributors) try to fix upcoming bugs, it'll be harder and harder to dive through various classes which sometimes have very similar names to figure out where something is done.

Having said that, we can surely merge this for now and simplify over time and as we get a better idea of what we want to get done.

> With gsoc-client-stuff, I'm working on asynchronously loading results, package
> info and hidden packages. This means implementing new views on the same query
> data as before. So, that's why I am a little picky about having one object to
> manage all that; all these views can just talk to the same thing in a somewhat
> balanced way. Having said that, it's mostly a stop-gap and if all goes well it
> should be really easy to yank it out for something quicker and more cool once
> the dust is settled. To be honest I'm not sure what the final result is going
> to look like in views.py, so I am reluctant to devote too much to a specific
> approach yet ;)

Gotcha. Thanks for your work on this and consideration.

review: Approve

« Back to merge proposal