[Freeipa-devel] [PATCH] 755 webui-ci: case-insensitive record check

[Petr Viktorin]

On 09/25/2014 03:30 AM, Fraser Tweedale wrote:
> On Wed, Sep 24, 2014 at 09:16:52AM -0500, Endi Sukma Dewata wrote:
>> On 9/24/2014 8:26 AM, Petr Vobornik wrote:
>>> On 24.9.2014 04:43, Endi Sukma Dewata wrote:
>>>> On 9/22/2014 9:49 AM, Petr Vobornik wrote:
>>>>> [PATCH] webui-ci: case-insensitive record check
>>>>>
>>>>> Indirect association are no longer lower cased, which caused a issue
>>>>> in CI.
>>>>
>>>> Is the use of "|=" operator intentional? I don't see the "has" variable
>>>> defined anywhere else in this method.
>>>>
>>>>    has |= self.has_record(pkey.lower(), parent, table_name)
>>>>
>>>> If this has been tested to work, then ACK.
>>>>
>>>
>>> it's defined on the previous line:
>>>
>>>    has = self.has_record(pkey, parent, table_name)
>>>    has |= self.has_record(pkey.lower(), parent, table_name)
>>
>> Ah, I see. I thought the old line was being replaced.
>> ACK.
>
> IMO the following reads better::
>
>      has = self.has_record(pkey, parent, table_name) \
>          || self.has_record(pkey.lower(), parent, table_name)
>
> (Just a comment - no reason to block the patch.)

Feel free to push the patch as it is, but since we're debating style...

Write this:

     has_record = (self.has_record(pkey, parent, table_name) or
                   self.has_record(pkey.lower(), parent, table_name))

For boolean values, you should prefer `or` and `and` over the bitwise 
operators, since "truthy"/"falsy" values might not be just booleans.
It's better illustrated with `and`: `3 and 8` is true, but `3 & 8` is false.


And from PEP8:
- The preferred way of wrapping long lines is by using Python's implied 
line continuation inside parentheses, brackets and braces. Long lines 
can be broken over multiple lines by wrapping expressions in 
parentheses. These should be used in preference to using a backslash for 
line continuation.

- The preferred place to break around a binary operator is after the 
operator, not before it.

-- 
Petr³