Thank you for patch, I have a few nitpicks:

On 10/08/15 13:05, Oleg Fayans wrote:
+def create_segment(master, leftnode, rightnode):
+    """create_segment(master, leftnode, rightnode)
Why do you add the name of method in docstring?


+def create_segment(master, leftnode, rightnode):
+    """create_segment(master, leftnode, rightnode)
+    creates a topology segment. The first argument is a node to run the 
command on
+    The first 3 arguments should be objects of class Host
+    Returns a hash object containing segment's name, leftnode, rightnode 
+    """

I would prefer to add assert there instead of just document that a Host object 
is needed
assert(isinstance(master, Host))

+def destroy_segment(master, segment_name):
+    """
+    destroy_segment(master, segment_name)
+    Destroys topology segment. First argument should be object of class Host

Instead of description of params as first, second etc., you may use following:

+def destroy_segment(master, segment_name):
+    """
+    Destroys topology segment.
+    :param master: reference to master object of class Host
+    :param segment: name fo segment
and eventually this in other methods
+    :returns: Lorem ipsum sit dolor mit amet
+    :raises NotFound: if segment is not found


cls.replicas[:len(cls.replicas) - 1],

I suggest cls.replicas[:-1]

In [2]: a = [1, 2, 3, 4, 5]

In [3]: a[:-1]
Out[3]: [1, 2, 3, 4]

Why re.findall() and then you just use the first result?
'leftnode': self.leftnode_re.findall(i)[0]

Isn't just enough?

Python 3 nitpick:
I'm not sure if time when we should enforce python 2/3 compability already 
comes, but just for record:
instead of open(file, 'r'), please use, 'r')  (import io before)

Martin Basti

Manage your subscription for the Freeipa-devel mailing list:
Contribute to FreeIPA:

Reply via email to