Request for patch review

[Jelmer Vernooij]

Hi Lukasz,

On Fri, 2012-10-12 at 10:16 +0100, Lukasz Zalewski wrote:
> On 12/10/12 09:53, Lukasz Zalewski wrote:
> > Hi all,
> > To put the new proposed rules for patch reviews to the test i would like
> > to ask for a review of patches that i have sent to the list (on
> > 25/06/12) with subject:
> > [PATCHES] Make add_remove_group_members a private helper function and
> > partial fixes for Bug #8891
> >
> > They are also attached to the Bug #8891.
> 
> Patches attached
Thanks for working on this.

These patches look good overall. Some quick comments:

* As Andrew says, some tests for the new methods would be really nice.

0002-samdb-Better-exception-handling-in-_add_remove_group:
* Please don't remove the FIXME's - they are still valid
* When raising errors in a cmd_* implementation, use CommandError rather
than Exception
* self.transaction_start() should be outside of the try/finally - if it
fails we don't want to abort the transaction

0003-s4-netcmd-group.py-throw-exception-when-nonexistent-.patch:
* When raising errors in a cmd_* implementation, use CommandError rather
than Exception

Cheers,

Jelmer