[Freeipa-devel] [PATCH] 316 Improve DN usage in ipa-client-install

[Martin Kosek]

On 10/02/2012 10:49 AM, Petr Viktorin wrote:
> On 09/27/2012 01:35 PM, Martin Kosek wrote:
>> A hotfix pushed in a scope of ticket 3088 forced conversion of DN
>> object (baseDN) in IPA client discovery so that ipa-client-install
>> does not crash when creating an IPA default.conf. Since this is not
>> a preferred way to handle DN objects, improve its usage:
>>
>> - make sure, that baseDN retrieved by client discovery is always
>>    a DN object
>> - update ipachangeconf.py code to handle strings better and instead
>>    of concatenating objects, make sure they are converted to string
>>    first
>>
>> As a side-effect of ipachangeconf changes, default.conf config file
>> generated by ipa-client-install has no longer empty new line at the
>> end of a file.
>>
>> https://fedorahosted.org/freeipa/ticket/3088
> 
> ACK, but since you're reformatting the code, here are some minor issues found
> by the PEP8 checker.
> 
>> diff --git a/ipa-client/ipaclient/ipachangeconf.py
>> b/ipa-client/ipaclient/ipachangeconf.py
>> index
>> f6288062be5c5d1b29341ed90814e1fa1431019c..2bb0cc487670ce74833fc09b06449d0a176ffaf5
>> 100644
>> --- a/ipa-client/ipaclient/ipachangeconf.py
>> +++ b/ipa-client/ipaclient/ipachangeconf.py
>> @@ -68,7 +68,7 @@ class IPAChangeConf:
>>           elif type(indent) is str:
>>               self.indent = (indent, )
>>           else:
>> -           raise ValueError, 'Indent must be a list of strings'
>> +           raise ValueError('Indent must be a list of strings')
> 
> Please re-indent to 4 spaces.
> 
>>
>> @@ -143,35 +143,50 @@ class IPAChangeConf:
>>       def getSectionLine(self, section):
>>           if len(self.sectnamdel) != 2:
>>               return section
>> -        return self.sectnamdel[0]+section+self.sectnamdel[1]+self.deol
>> +        return self._dump_line(self.sectnamdel[0],
>> +                               section,
>> +                               self.sectnamdel[1],
>> +                               self.deol)
>> +
>> +    def _dump_line(self, *args):
>> +        return u"".join(unicode(x) for x in args)
>>
>>       def dump(self, options, level=0):
>> -        output = ""
>> +        output = []
>>           if level >= len(self.indent):
>>               level = len(self.indent)-1
>>
>>           for o in options:
>>               if o['type'] == "section":
>> -                output +=
>> self.sectnamdel[0]+o['name']+self.sectnamdel[1]+self.deol
>> -                output += self.dump(o['value'], level+1)
>> +                output.append(self._dump_line(self.sectnamdel[0],
>> +                                              o['name'],
>> +                                              self.sectnamdel[1]))
>> +                output.append(self.dump(o['value'], level+1))
> 
> Please surround the + operator by spaces (here & below).
> 
>> @@ -274,15 +297,19 @@ class IPAChangeConf:
>>                           opts.append(o)
>>                           continue
>>                       if no['action'] == 'comment':
>> -                       opts.append({'name':'comment', 'type':'comment',
>> -                                   
>> 'value':self.dcomment+o['name']+self.dassign+o['value']})
>> +                        value = self._dump_line(self.dcomment,
>> +                                                o['name'],
>> +                                                self.dassign,
>> +                                                o['value'])
>> +                        opts.append({'name':'comment', 'type':'comment',
>> +                                     'value':value})
> 
> Please add a space after each colon.
> 

Since the whole ipachangeconf.py is not much PEP8 compliant, I did not want to
create just small "islands of compliance" with different coding style that the
rest of the file.

In attached patch I fixed the PEP8 warnings in the whole file.

Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mkosek-316-2-improve-dn-usage-in-ipa-client-install.patch
Type: text/x-patch
Size: 18327 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20121002/280ecfc8/attachment.bin>