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. -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code