On 11/14/2013 12:05 PM, Roberto Polli wrote:
Hi Thierry,
my consideration follows (a github-like platform with inline comments would be
really welcome)!
= method naming and placement =
1- I would use the following convention: if a method setup a functionality
adding various entries to the tree, I would name it "setup" and hopefully
should be placed DSAdminTools.
Hi Roberto,
Sorry for this late feedback.
I agree that 'setup' is commonly used to prepare/initialize a
functionality. Now I would prefer verbs of action/unaction like
create/delete, enable/disable, set/get/list. With 'setup' verb we may
create entries, enable functionality, set properties. If we have a
function setupAgreement (that creates the RA and enables it by default)
what is the name for the function that delete the agreement
'deleteAgreement' ?
So far DSAdmintools mainly contains offline functions (like start/stop
instance). I agree we can put setup functions in it. But I wonder if it
would be interesting to keep all offline functions in a separated file.
2- methods like _createDefaultReplMgr are not expected to be used in
production, so should be placed in the DSAdminTools section
I am not sure. If someone wants to rapidly deploy a replication
topology, he would be interested to have a default replication manager.
In that case we may offer 'createDefaultReplMgr' (without heading '_').
In your opinion what kind of functions would go into DSAdminTools ? all
'setupxxx' functions ?
3- the brooker naming convention is based on the "function-first" so that
python interactive users can tab and autocomplete it.
initAgreement should be renamed to something like agreement_init or something
else. For the return codes, simply use exceptions.
ok. So do you prefer names like 'replica_create', 'suffix_create',
'agreement_create', rather than 'createReplica', 'createSuffix',
'createAgreement' ? Ok I will change the name.
= exception handling & None return =
1- in case of errors, a method should raise a proper exception and eventually
log the error
2- so the assert clauses should be replaced by exception because they mean
that something went wrong and an action should be taken
3- about the bindmethod stuff: see http://pastebin.com/w0WnVQuJ
Absolutely, I will change the error handling. In addition, exception
makes most of the time the code easier to read.
Thanks
I will resend a review according to you suggestions.
regards
thierry
There are some other points but the best way to set them is with patches.
Let me know + Peace,
R.
On Wednesday 13 November 2013 17:06:25 thierry bordaz wrote:
In order to implement the first CI test with replication instances, I
reorganised lib389 functions related to replication setup.
https://fedorahosted.org/389/attachment/ticket/47590/0001-Ticket-47590-CI-te
sts-add-split-functions-around-rep.patch
--
389-devel mailing list
389-devel@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/389-devel
