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

[Oleg Fayans]

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.
>
>
> 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

>
> 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? :)

>
>
> 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.
>
>
>

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