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
169.
For reasons I said before I'd prefer if single_value stayed a simple
function.
IMO a view better matches its semantics, plus I changed the code, so I
would like to keep it a view, if you don't mind.
OK, ACK to that one as well, but I'd rather wait a few weeks (until 3.3
churn dies out) before pushing it, since it could complicate backporting
patches.
--
Petr³
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
