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 _______________________________________________ Freeipa-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/freeipa-devel
