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

[Martin Basti]

On 08/11/2015 03:36 PM, Oleg Fayans wrote:
> Hi Martin,
>
> On 08/11/2015 02:02 PM, Martin Basti wrote:
>> 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
>>
> Fixed, sorry.
>>
>>>>
>>>>
>>>> 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.
>
> Agreed. Modified the decorator so that you can specify a slice of 
> arguments to be checked and a class to compare to. This does scale :)
>>
>> 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.
>
> I got it. Fixed.
>
>>
>>
>>>
>>>>
>>>>
>>>> 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)
>
> Removed
>>
>

NACK

you changed it wrong

group() returns everything, you need use group(1) to return content in 
braces.