On 10/02/2012 01:33 PM, Petr Viktorin wrote: > On 10/02/2012 12:48 PM, Martin Kosek wrote: >> 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 >> > > Looking better. ACK >
Pushed to master, ipa-3-0. Martin _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
