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

[Fraser Tweedale]

On Thu, Sep 25, 2014 at 09:44:03AM +0200, Petr Viktorin wrote:
> 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))
> 
Hah, yes.  What I wrote is not even Python!  I have been writing too
much Java it seems -_-

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