[Freeipa-devel] [PATCH] 0201 Add support for an external trust to Active Directory domain

[Alexander Bokovoy]

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