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

[Oleg Fayans]

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
>

-- 
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0002.4-Integration-tests-topology-plugin.patch
Type: text/x-patch
Size: 12100 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150811/58a5f490/attachment.bin>