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:
On 09/26/2013 02:22 PM, Jan Cholasta wrote:
On 24.9.2013 15:35, Jan Cholasta wrote:
On 27.2.2013 16:31, Jan Cholasta wrote:
Hi,
these patches add the ability to access and manipulate raw attribute
values as they are returned from python-ldap to the LDAPEntry class.
This is useful for comparing entries, computing modlists for the
modify
operation, deleting values that don't match the syntax of an
attribute,
etc., because we don't need to care what syntax does a particular
attribute use, or what Python type is used for it in the framework
(raw
values are always list of str). It should also make interaction with
non-389 DS LDAP servers easier in the framework.
(It might be too late for this kind of changes to get into 3.2 now,
I'm
posting these patches mainly so that you are aware that they exist.)
Honza
This is now planned for 3.4:
<https://fedorahosted.org/freeipa/ticket/3521>
I fixed some issues in these patches and refined the API. Updated
patches attached.
Also added a patch to use raw values when adding new entries and a
patch
which refines LDAPEntry.single_value, so that it is consistent with
the
rest of the changes introduced in the patches.
Patch 110 will probably be dropped in favor of Petr Viktorin's schema
update patches, but I included it anyway.
Incidentally, this also fixes
<https://fedorahosted.org/freeipa/ticket/3927> and possibly also
<https://fedorahosted.org/freeipa/ticket/2131>.
Honza
Noticed a couple more issues and fixed them. Updated patches attached.
Honza
Thanks for the patches!
106. Make LDAPEntry a wrapper around dict rather than a dict subclass.
Git rants about one more whitespace error.
[...]
+ def __eq__(self, other):
+ if not isinstance(other, LDAPEntry):
+ return NotImplemented
+ if self._dn != other._dn:
+ return False
+ return super(LDAPEntry, self).__eq__(other)
I don't think equality checking makes sense on a LDAPEntry, where you
might have different capitalizations/variants of attribute names,
different _orig, or a different set of attributes loaded on the same
entry. It's not obvious which of those differences should make the
entries inequal.
I'd just base it on identity (`self is other`).
Right, I'm not sure why I even did it this way. But I remember seeing
some code that did comparison of entries with ==...
Thanks.
Please also implement __ne__() when reimplementing __eq__().
def __iter__(self):
yield self._dn
yield self
This makes the whole thing sort of hackish -- we need to reimplement
everything in MutableMapping that uses iter() internally :(
Hopefully we can get rid of it soon.
Yes, it's a shame MutableMapping uses iter() instead of iterkeys().
I'd welcome FIXME comments on whatever is reimplemented for this reason.
I thought the comment above __iter__ would be enough. Apparently I was
wrong.
Right, IMO it's not immediately obvious that these are reimplemented
because they depend on __iter__.
[...]
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 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!
169.
For reasons I said before I'd prefer if single_value stayed a simple
function.
--
Petr³
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
