Review for python patch for upgradeprovision

[Matthieu Patou]

  Hi Jelmer,
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 ?
>>
>> 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..
>
Well it's was more about the style, the result is normally ok I tortured 
several provision
> 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 
increment_calculated_keyversion_number
     val = e.get("msDs-KeyVersionNumber", "0")
TypeError: function takes exactly 1 argument (2 given)
0

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
>
> Cheers,
>
> Jelmer
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.


Matthieu.

-- 
Matthieu Patou
Samba Team        http://samba.org