[Freeipa-devel] DN patch and documentation

[John Dennis]

On 07/27/2012 11:18 AM, Petr Viktorin wrote:
> The nitpicks continue. None of the things in this mail are holding up
> the patch, consider it more of a discussion about good practices. (I'm
> assuming you're interested.)
>
>
>
> ipaserver/rpcserver.py:
>        elif isinstance(val, (list, tuple)):
> -        new_list = []
> -        n = len(val)
> -        i = 0
> -        while i < n:
> -            v = val[i]
> -            if isinstance(v, str):
> -                new_list.append({'__base64__' : base64.b64encode(v)})
> -            else:
> -                new_list.append(json_encode_binary(v))
> -            i += 1
> +        new_list = [json_encode_binary(v) for v in val]
>            del val
>            return new_list
>
> You might as well remove the `del val`s.

Agreed, it baffled me as to why they were there in the first place. 
There are few other del's that need to be removed as well. I made the 
change.

>
> ipapython/dn.py:
> -Concatenation and In-Place Addition:
> +Concatenation, In-Place Addition, Insetion:
>
> s/Insetion/Insertion/

Fixed.

> In the giant comment in ipapython/dn.py:
> ...
> But in general I think lengthy explanations of Python's behavior ...

Text is removed.

> By the way, are you aware that the text in dn.py is not a docstring,
> because it's not the first statement in the file?

Good catch, no I missed that.

>>> Line 107 & 127:
>>>        return utf8_codec.decode(val)[0]
>>>        return utf8_codec.encode(unicode(val))[0]
>>> I believe the following would be more readable:
>>>        return val.decode('utf-8')
>>>        return unicode(val).encode('utf-8')
>>
>> Yes, your suggestion is easier to read, but it's more efficient to
>> lookup the codec once rather than everytime you decode.
>
> I wouldn't worry about that; lookups are pretty fast -- I'd say
> comparable to creating an unnecessary tuple. Of course it's hard to know
> without profiling.

Fair enough, it's been changed.

>>> Line 354:
>>>        _SCHEMA_OVERRIDE = {
>>> We have a case-insensitive dict class, why not use it here?
>>
>> Excellent suggestion, it's been changed.
>
> Please also change the use:
>           attr_lower = attr.lower()
>           syntax = self._SCHEMA_OVERRIDE.get(attr_lower)
> to
>           syntax = self._SCHEMA_OVERRIDE[attr]

Good catch, fixed.

>>> Line 406:
>>>            if isinstance(val, bool):
>>>                if val:
>>>                    val = 'TRUE'
>>>                else:
>>>                    val = 'FALSE'
>>>                return self.encode(val)
>>> Encoding an ASCII string should be a no-op, why bother doing it?
>>
>> Not sure what you mean by no-op. Do you mean str(val) would return
>> either 'True' or 'False'?
>>
>> In any event we are dependent on the string representation of a boolean
>> being all uppper-case. Not something I invented.
>
> I meant the `return self.encode(val)`, with val being either 'TRUE' or
> 'FALSE'. The following should suffice:
>
>              if isinstance(val, bool):
>                  if val:
>                      return 'TRUE'
>                  else:
>                      return 'FALSE'
>
> Sorry for not being clear.

Duh, John smacks his forehead :-) Fixed.

>>> Line 421:
>>>        dct = dict()
>>>        for (k, v) in val.iteritems():
>>>            dct[self.encode(k)] = self.encode(v)
>>>        return dct
>>> Consider:
>>>        dct = dict((self.encode(k), self.encode(v)) for k, v in
>>> val.iteritems())
>>
>> Why do the work of building a list of tuples when it's not necessary?
>
> It's a generator expression, it doesn't build a list.

Good point, changed.

>>> Line 1422:
>>>        self.handle_errors(e, **{})
>>> Wouldn't the following work better?
>>>        self.handle_errors(e, *[{}]*0)
>>
>> I never liked the way handle_errors was written. The use of the kwds
>> args is odd, convoluted and hard to understand.
>>
>> To be honest I can't parse
>>
>> *[{}]*0
>
> Sorry, lame attempt at humor.
> The **{} doesn't do anything at all, you can remove it.
> The keyword arguments in handle_errors are unused, so you can remove
> that as well if you want.

Cruft from previous implementation, it's been removed.



-- 
John Dennis <jdennis at redhat.com>

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/