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

[Tomas Babej]

On 08/13/2015 05:06 PM, Martin Basti wrote:
> 
> 
> 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.
> 

I'd suggest using named groups in this case, it leads to clearer
expressions.

See: https://docs.python.org/2/library/re.html , (?P<name>...) section.