Review for python patch for upgradeprovision

Jelmer Vernooij jelmer at
Fri Aug 13 15:53:25 MDT 2010

Hi Matthieu!

On Sat, 2010-08-14 at 01:37 +0400, Matthieu Patou wrote:
> Could you have a look at this patches ?

This review covers the python side of things mostly, I'm not too
familiar with the ins and outs of upgrading. That said.. 

Nice branch! Some minor (emphasis on minor) nitpicks:

 * in 11118e0e89652b9545dbcabf572c583554b7c797 you add a print statement
to - was that intentional or just for debugging? I
don't think helper functions should write to stdout, so they can e.g. be
used by guis or by tests while still being useful to the user.
 * you can give a default value to dict.get(), which will be used if the
key was not found. Rather than:

                 val = e.get("msDs-KeyVersionNumber")
                 if not val:
                     val = "0"

You can also use:

		val = e.get("msDs-KeyVersionNumber", "0")

* This seems a bit strange:

+    for ent in current:
+        schema_ldif = "%s%s" % (schema_ldif,
+                        samdb.write_ldif(ent, ldb.CHANGETYPE_NONE))

More logical seems:
     for ent in current:
         schema_ldif += samdb.write_ldif(ent, ldb.CHANGETYPE_NONE)

Or, if you're a fan of list comprehensions:

     schema_ldif += "".join([samdb.write_ldif(ent, ldb.CHANGETYPE_NONE)
for ent in current])

* The docstring of int64range2str has an unnecessary leading space


-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <>

More information about the samba-technical mailing list