[Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin

[Martin Basti]

NACK, comments inline.

On 11/08/15 13:25, Oleg Fayans wrote:
> Hi Martin,
>
> Thanks for the review!
>
> On 08/10/2015 07:08 PM, Martin Basti wrote:
>> Thank you for patch, I have a few nitpicks:
>>
>> 1)
>> 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?
> My bad, fixed.

still there

+        tokenize_topologies(command_output)
+        takes an output of `ipa topologysegment-find` and returns an array of


>>
>>
>> 2)
>>
>> +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 information
>> +    """
>>
>> I would prefer to add assert there instead of just document that a Host
>> object is needed
>> assert(isinstance(master, Host))
>> ...
>
> Fixed. Created a decorator that checks the type of arguments

This does not scale well.
If we will want to add new argument that is not host object, you need 
change it again.

This might be used as function with specified variables that have to be 
host objects
>
>>
>> 3)
>> +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
>>
> Fixed
>>
>> 4)
>>
>> 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]
>>
> Fixed
>>
>> 5)
>> Why re.findall() and then you just use the first result?
>> 'leftnode': self.leftnode_re.findall(i)[0]
>>
>> Isn't just re.search() enough?
>> leftnode_re.search(value).group(1)
>
> in fact
> leftnode_re.findall(string)[0]
> and
> leftnode_re.search(string).group(1),
> Are equally bad from the readability point of view. The first one is 
> even shorter a bit, so why change? :)

It depends on point of view,  because when I reviewed it yesterday my 
brain raises exception that you are trying to add multiple values to 
single value attribute, because you used findall, I expected that you 
need multiple values, and then I saw that index [0] at the end, and I 
was pretty confused what are you trying to achieve.

And because findall is not effective in case when you need to find just 
one occurrence.


>
>>
>>
>> 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 io.open(file, 'r') (import io
>> before)
>>
> Done.
>>
>>
>>
>

1)

+#

empty comment here (several times)

-- 
Martin Basti