Review for python patch for upgradeprovision

[Jelmer Vernooij]

Hi Matthieu!

On Sat, 2010-08-14 at 01:37 +0400, Matthieu Patou wrote:
> Could you have a look at this patches ?
> 
> http://git.samba.org/?p=mat/samba.git;a=shortlog;h=refs/heads/upgradeprovision-review

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 upgradehelpers.py - 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

Cheers,

Jelmer
-------------- 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: <http://lists.samba.org/pipermail/samba-technical/attachments/20100813/2f37467d/attachment.pgp>