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

[Petr Viktorin]

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.

-- 
Petr³