On 02/04/2013 02:25 PM, Jan Cholasta wrote:
On 1.2.2013 12:12, Petr Viktorin wrote:
On 01/31/2013 04:18 PM, Jan Cholasta wrote:
Hi,

these patches implement attribute name case preservation in LDAPEntry.
Apply on top of Petr Viktorin's LDAP code refactoring patchset (up to
part 5).

Honza

Patches 99 & 101 need some tests to make sure the _names work correctly.

Added.

I see one of the changes is using has_key instead of `in` for a CIDict. Given that dict.has_key() is deprecated, I think a better solution would be to add __contains__ to CIDict. Is there a reason against that?

Since you can call LDAPEntry.__init__ in different ways which don't
always correspond to the argument names, it would be nice to explain
them in a docstring.

Done.


A few nitpicks below.

freeipa-jcholast-99-Preserve-case-of-attribute-names-in-LDAPEntry.patch

diff --git a/ipaserver/ipaldap.py b/ipaserver/ipaldap.py
index 20c11b4..6268ac0 100644
@@ -650,14 +637,48 @@ class LDAPEntry(dict):
          self._orig = self
          self._orig = deepcopy(self)

+    def _attr_name(self, name):
+        if not isinstance(name, (unicode, str)):

Use basestring instead of (unicode, str).

Is there any compelling reason to do so? I don't want to support
arbitrary basestring subclasses in this code, just unicode and str.

Using basestring is the standard idiom. Since you're supporting arbitrary str and unicode subclasses anyway, I don't see your point. Besides, basestring can't even be instantiated. Someone who'd subclass basestring directly would really have to know what they're doing.

freeipa-jcholast-100-Aggregate-IPASimpleLDAPObject-in-LDAPEntry.patch

diff --git a/ipaserver/ipaldap.py b/ipaserver/ipaldap.py
index 6268ac0..01fa0c1 100644
@@ -595,8 +600,10 @@ class LDAPEntry(dict):
                  _obj = {}
              orig = None

+        assert isinstance(_conn, IPASimpleLDAPObject)
          assert isinstance(_dn, DN)

+        self._conn = lambda: _conn  # do not deepcopy me!

This would be better done by overriding __deepcopy__, or just using a
custom method for the copying.


I have tried multiple different solutions and none is as elegant as
this. Yes, it is a hack, but since it is an internal implementation
detail of LDAPEntry, I don't see any harm in doing it.

On further thought, custom method is probably better suited for this job
than than deepcopy. Added.

Updated patches attached.

Could you also add a docstring to commit()? The function is not clear from the name alone.

Other than that, the patches look good.

--
PetrĀ³

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to