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 <jden...@redhat.com>
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
