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

Thanks a lot for your continued work on this and sorry for not replying earlier. I've been quite busy with other things. Sorry.

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

In cases where setters and getters just do the minimal amount of work ("self.bla = bla" and "return bla"), I'd expect that accessing the object's property would just work. If additional work needs to be done, I'd try to do stuff in the constructor or in a function that does some kind of computation or other work.

I don't know if that's applicable here.

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

Can you maybe explain what kind of summary you'd like to see there?

I personally feel it's a bit more work to get it right, but it might be beneficial to tune the queries in views.py right, which would also save us from loading too many objects into memory.

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

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.

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

You're a true rockstar! Let's have a longer chat soon again. :)