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

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.


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
--- 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, )
-           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 +=
-                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:
                       if no['action'] == 'comment':
-                       opts.append({'name':'comment', 'type':'comment',
+                        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.


Looking better. ACK


Freeipa-devel mailing list

Reply via email to