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

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
set-like, since the order doesn't matter.



Updated patches attached.

It can't hurt to have this in for now.

111 - 121 look great!

106 - 121: ACK

For reasons I said before I'd prefer if single_value stayed a simple

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.


Freeipa-devel mailing list

Reply via email to