On Monday 18 November 2013 11:08:01 thierry bordaz wrote: > Hi, this is a second review to take into account the recommendation of > the first review. > Major changes are: > > * Create a replicaagreement class in brooker > * mv init/status/schedule/create in that new class > * mv createDefaultReplMgr into the brooker replica class > * Handling of error condition with exceptions > > https://fedorahosted.org/389/attachment/ticket/47590/0002-Ticket-47590-CI-te > sts-add-split-functions-around-rep.patch 1- Here and elsewhere: if not suffix: # This is a mandatory parameter of the command... it fails log.warning("createAgreement: suffix is missing") - return None + raise ValueError("suffix is a required value") 2- The method "enableReplication" should probably go into the Replica class, and be called like conn.replica.enable() 3- enableReplication and agreementInit should raise in case of errors, not return 1 or -1. Managing error codes is extra work: and moreover which value is fine? Greater than zero? Zero? True? 4- If you put replica_createReplMgr in the Replica brooker, just name it create_manager() so that we can call it conn.replica.create_manager(). The replica is inferred from the context! 5- If you move the agreement stuff outside Replica, I would just name the class "Agreement" and set self.agreement = Agreement(self) or self.replica_agreement -- Roberto Polli Community Manager Babel S.r.l. - http://www.babel.it T: +39.06.9826.9651 M: +39.340.652.2736 F: +39.06.9826.9680 P.zza S.Benedetto da Norcia, 33 - 00040 Pomezia (Roma) CONFIDENZIALE: Questo messaggio ed i suoi allegati sono di carattere confidenziale per i destinatari in indirizzo. E' vietato l'inoltro non autorizzato a destinatari diversi da quelli indicati nel messaggio originale. Se ricevuto per errore, l'uso del contenuto e' proibito; si prega di comunicarlo al mittente e cancellarlo immediatamente. -- 389-devel mailing list 389-devel@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/389-devel
