[389-devel] Please review lib389 ticket 47568: Rename DSAdmin class (2nd)

[thierry bordaz]

On 10/25/2013 10:19 AM, Roberto Polli wrote:
> My comments (a github like platform for comment could be really useful)
>> https://fedorahosted.org/389/attachment/ticket/47568/0002-Ticket-47568-Renam
>> e-DSAdmin-class.patch
> line: comment
> lib389/__init__.py:1: the module is lib389, not dirsrv
Thanks Roberto for your review

Yes right I will change it
>
> lib389/brooker.py:795: python variable naming convention: I would get stick
> with the "_" instead of camelCase  and change whenever possible.
It is a good time to decide convention that would help to read the code.
I was not able to find a clear convention for local variables. For 
instance variable yes I think it is recommended to use '_'
If you prefer to use '_' also for local variable, I am fine. I will 
change according to this.
>
> tests/dsadmin_test.py: I renamed it lib389_test.py, you can merge my changes
> tests/dsadmin_test.py:39: why remove the addbackend_harn?
Humm, to be honest... I do not know how to rename files :-) . I have 
seen you did it in 
https://github.com/ioggstream/dsadmin/compare/merge_lib389, how did you 
did that ?
Yes I agree it will not be used but I was a bit afraid to get rid of 
already written functions
>
> tests/replica_test.py:119: you're using Backend.delete in a class that should
> test just Replica. I would use harness and the standard python-ldap methods in
> setup/teardown, so that we can change the Backend and Replica class without at
> least breaking the tests.
I miss your point. It is calling in teardown conn.backend.delete, is 
that the call that is not correct ?


regards
thierry
>
> Let me know + Peace,
> R.
>