[Piuparts-devel] faster-rdep-calc

[Andreas Beckmann]

On 2013-03-16 16:18, Dave Steele wrote:
> On Sat, Mar 16, 2013 at 6:36 AM, Andreas Beckmann <anbe at debian.org> wrote:
>>
>> I just noticed that (in the version I merged some time ago) depcount is
>> now always 1-based (probably including the package itself) and no longer
>> 0-based as before.
> 
> Hmm, yes. That is a side effect of replacing the recursive function
> for collecting rdeps.

Can you fix this?

> Are you still interested in these branches?

Yes :-) Can you rebase them?

some older comments:

>piuparts-report needs to be fixed wrt. calling load_package_database,
>master_directory must be passed on
>
>the chnagelog entry has a mistyped "puparts"
>
>sectiondir = os.path.join( master_directory, section)
>bad spacing:              ^

and some new ones:

patches 1-3: OK

patch 4:

-        self._rrdep_count = None
+        self.rrdep_count = None

     def rrdep_count(self):
-        if self._rrdep_count == None:
+        if self.rrdep_count == None:
             raise something
-        return(self._rrdep_count)
+        return(self.rrdep_count)

Hmm, renaming the variables to the same name as the methods - makes my
brain hurt. Does that even work? The accessor methods will get removed
in patch #7 ... so it's not a problem in the final result, but I'd like
to avoid adding broken intermediate commits

in Package, having accessors to set the value (set_foo_count()), but
requiring to access the member variable directly to get the vlue looks
like a stupid assymmetry.
So I'd suggest
* leave the variables as self._foo_count
* keep the foo_count methods, but drop the test for None and don't raise
anything - the caller needs to handle None (and does this)

so PackagesDB should have

+    def foo_count(self, pkg):
+        if pkg.foo_count() is None:
+            self.calc_rrdep_pkg_counts(pkg)
+
+        return pkg.foo_count()

also note "is None", IIRC this is generally preferred over "== None"
(and in the current code we have 3 "==" vs. 24 "is" and 24 "is not")

Haven't looked in detail into patches 5+, yet, but both series are
running in my instance :-)


Andreas