On Tue, 07 Jun 2016, Martin Babinsky wrote:
Again, we only require contributors to follow PEP8 when adding new code/directly touching old one. Please note that there are more serious transgressions than a couple of long lines that should _definitely_ be fixed (indentation errors, whitespace around operators etc.).

We as a contributors to the project _all_ have to conform to the style guide[1] to at least keep some uniform style in the codebase we touch. I fail to see why your contributions should be exempt from the rules we all strive to adhere to.
Sure we do. It is futile to shuffle dcerpc.py into pep8 style, though.
Look into an example I provided to mbasti, for example:

           res['ipantflatname'] = 
unicode(t.forest_trust_data.netbios_domain_name.string)

This line is 90+ characters long and there are not that many methods to
split it without making thing ugly. I'm not against formatting changes
but they should be taken in the context of the code.


(also you can sandwich # pylint: disable/ # pylint: enable directives around the offending long live to keep pep8 satisfied, just saying)

[1] http://www.freeipa.org/page/Python_Coding_Style


2.)

I have difficulty understanding the following code:

"""
+        self.__allow_behavior = 0

       domain_validator = DomainValidator(api)
       self.configured = domain_validator.is_configured()

       if self.configured:
           self.local_flatname = domain_validator.flatname
           self.local_dn = domain_validator.dn
           self.__populate_local_domain()

+    def allow_behavior(self, behavior_set):
+        if type(behavior_set) is int:
+           behavior_set = [behavior_set]
+        if TRUST_JOIN_EXTERNAL in behavior_set:
+           self.__allow_behavior |= TRUST_JOIN_EXTERNAL
+
"""
I assume this is made like this to accommodate setting some other
behavior flags which can pop up in the future (otherwise a simple
Boolean to indicate external trust should be enough), int hat case I
would propose to rewrite it in this form and document it:


"""
def allow_behavior(self, *flags):
  """
  Set the expected trust join behavior
  :param flags: trust flags to set
  """
  for flag in flags:
      self.__allow_behavior |= flag
"""

Also, I am not a big fan of doubly underscored methods/variables since
they do not 'hide' anything (Python's name mangling can be bypassed
with ease anyway). If you want to indicate that the 'allow_behavior'
attribute belongs to the internal implementation of the class prefix
it with single underscore.
You cannot have a property and a method named the same in the same
class. I changed the code to follow your other suggestion. I kept the
__allow_behavior as it is, though, because there is no difference
between __ and _ semantically to a person who would be reading the code
in question but I keep __ memorized. ;)


Fair enough, although you could have more meaningful method and attribute names, like `self.__behavior_flags` and `def set_behavior_flags()`.
Nah, this is not worth it, really.

Updated patch is attached.


Also when sending revised patches please try to conform to the common patch naming convention: http://www.freeipa.org/page/Contribute/Patch_Format#Naming

Surprisingly I have found out that my own patches actually do not follow it. Shame on me!
Frankly speaking, this particular part -- revision number after the
patch number versus at the end of the patch file name -- always created
issues to me. You can tell git to use specific suffix (.1.patch, for
example) to avoid doing full renames with the latter rather than
shoehorning the number into the middle.

--
/ Alexander Bokovoy

--
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

Reply via email to