Review for python patch for upgradeprovision
mat at samba.org
Sat Aug 14 09:10:33 MDT 2010
On 14/08/2010 01:53, Jelmer Vernooij wrote:
> 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..
Well it's was more about the style, the result is normally ok I tortured
> 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"
I tried and got:
File "bin/python/samba/upgradehelpers.py", line 741, in
val = e.get("msDs-KeyVersionNumber", "0")
TypeError: function takes exactly 1 argument (2 given)
It seems that the the get method in the ldb.Message object didn't
support the things that a dictionnary support.
> 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
So other than the val = e.get("msDs-KeyVersionNumber", "0") I made the
different change that you proposed (they are all in my branch now).
I added a few patches for small pbs that I found also during testing.
Samba Team http://samba.org
More information about the samba-technical