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.
--
389-devel mailing list
389-devel@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/389-devel
