[Freeipa-devel] [PATCHES] 106-113 Access raw LDAP values directly from LDAPEntry

[Martin Kosek]

On 10/29/2013 04:17 PM, Petr Viktorin wrote:
> On 10/29/2013 01:34 PM, Jan Cholasta wrote:
>> On 16.10.2013 18:13, Petr Viktorin wrote:
>>> On 10/14/2013 10:59 AM, Jan Cholasta wrote:
>>>> On 10.10.2013 09:45, Jan Cholasta wrote:
>>>>> On 9.10.2013 13:57, Petr Viktorin wrote:
> [...]
>>>>>> 109. Decode and encode attribute values in LDAPEntry on demand.
>>>>>>
>>>>>> The syncing looks rather over-engineered to me.
>>>>>> Did you consider a custom MutableSequence for the values?
>>>>>> I think it would be much cleaner in the end than merging two sets of
>>>>>> changes together.
>>>>>
>>>>> I'm not entirely happy about it either, but it works. I did consider a
>>>>> custom sequence type, but I didn't feel like it was the right time to
>>>>> this kind of change in this patchset.
>>>>>
>>>>> Unlike the (DN, dict) -> LDAPEntry
>>>>> transition, this change won't be backward compatible and there is a lot
>>>>> of isinstance(value, list) and entry[attr] = list() kind of things in
>>>>> the framework code.
>>>
>>> That's what I was afraid of.
>>>
>>> We could live with `isinstance(value, list)`; hopefully we could get rid
>>> of `type(value) == list` that is the real problem.
>>> With `entry[attr] = list()` we could convert automatically.
>>>
>>> But OK, let's settle for a worse solution in the meantime.
>>>
>>>
>>> To be frank I don't particularly like the LDAPEntryView.
>>> While the dict-like interface is great, there isn't a case for storing a
>>> Raw view long-term, i.e. you'd always want to do something like
>>>      values = entry.raw[x]
>>>      ...
>>>      entry.raw[x] = new_values
>>> rather than
>>>      raw = entry.raw
>>>      values = raw[x]
>>>      ...
>>>      raw[x] = new_values
>>> The latter is confusing because LDAPEntryView and RawLDAPEntryView are
>>> two classes that have exactly the same interface, but do something
>>> different. In a duck-typed world that's a recipe for disaster.
>>> I think it would be better if the view implemented just the dict
>>> protocol, and not `conn`, `dn`, `nice`, `raw`, etc.
>>> The code would also be much simpler without the elaborate view class
>>> hierarchy.
>>>
>>> If you don't agree then at least don't make `raw` available on raw views
>>> and `nice` on nice views; the programmer *always* needs to know which
>>> version they're working with so these aren't necessary.
>>
>> I agree. Most of the attributes are leftovers from a previous
>> implementation, which didn't work very well. I should have removed them
>> a long time ago. Thanks for pointing this out!
>>
>> Updated the views to provide only the dict interface, removed "nice" and
>> "multi_value" properties and also removed "single_value" from the raw view.
> 
> Looks much better now. Hopefully _sync_attr can dissappear one day.
> 
>>>>>> I think it would also help (in the future?) to make the value lists
>>>>>> more
>>>>>> set-like, since the order doesn't matter.
>>>>>
>>>>> +1
>>>>>
>>>>> Honza
>>>>>
>>>>
>>>> Updated patches attached.
>>>>
>>>
>>> 110.
>>> It can't hurt to have this in for now.
>>>
>>> 111 - 121 look great!
> 
> 106 - 121: ACK

106-121 pushed to master.

Martin