[Interface-dev] interface.py
Thomas Lotze
tl at gocept.com
Sun Jul 10 17:35:52 EDT 2005
Hi,
I've taken a closer look at the implementation of interfaces in
interface.py recently and noticed a couple of (probably minor) issues.
- First of all, I found several places where functionality built into
Python is reimplemented, e.g. dict.setdefault or dict.pop. I've tried
out some changes, and as the same tests break now as did before ;o), I
assume my changed code is equivalent to that from the repository.[1]
I've included a patch against
$Id: interface.py 29899 2005-04-07 17:44:44Z jim $ below. It also
includes some changes that concern readability only (at least I hope
my version is more readable...).
- I think it would be good to expose the tagged values dict on Elements
directly, as an Attribute of IElement. Right now, the *TaggedValue*
methods forward part of the dict interface, in a way that doesn't add
any functionality nor semantics but hides a lot. (Just look at those
places in interface.py where loops are needed to set several tagged
values from a dict.) Plus, queryTaggedValues is to getTaggedValue as
dict()[key] is to dict.get with regard to the default parameter, which
is a confusing naming convention.
- It would probably be a good idea for Interface.namesAndDescriptions
(or another method for the sake of backwards compatibility) to return
a dict mapping names to descriptions instead of a list of pairs. It's
much more natural for the user to call items() on a dict if so desired
than to reconstruct a dict from the items. It would also eliminate the
need for Interface.names and Interface.{get,query}DescriptionsFor.
- Using a closure to implement InterfaceClass.__call__ is not IMO an
evil trick, it's a useful feature of the language.
I'd like to hear your opinions on these issues.
Thomas
[1] There's actually one place where I wittingly changed behaviour in a
way which IMO shouldn't matter: InterfaceClass.__init__ used to have a
default argument attrs=None, which would be set to {} later. I think
having attrs={} right as the default is the better solution. The change
in behaviour is that an AttributeError occurs if attrs is explicitly set
to None in a call - if that shall be possible, this part of the patch
would have to be skipped. The method would break anyway if some nonsense
(any non-dict, non-None object) was passed as attrs.
36,41c36,37
< tags = f_locals.get(TAGGED_DATA)
< if tags is None:
< tags = f_locals[TAGGED_DATA] = {}
< invariants = tags.get('invariants')
< if invariants is None:
< invariants = tags['invariants'] = []
---
> tags = f_locals.setdefault(TAGGED_DATA, {})
> invariants = tags.setdefault('invariants', [])
359,363c355
< if callback is None:
< return weakref.ref(self)
< else:
< return weakref.ref(self, callback)
<
---
> return weakref.ref(self, callback)
393c385
< def __init__(self, name, bases=(), attrs=None, __doc__=None,
---
> def __init__(self, name, bases=(), attrs={}, __doc__=None,
397,401c389,390
< if (attrs is not None and
< ('__module__' in attrs) and
< isinstance(attrs['__module__'], str)
< ):
< __module__ = attrs['__module__']
---
> __module__ = attrs.get('__module__')
> if isinstance(__module__, str):
404d392
<
415,417d402
< if attrs is None:
< attrs = {}
<
430,434c415
< if attrs.has_key(TAGGED_DATA):
< tagged_data = attrs[TAGGED_DATA]
< del attrs[TAGGED_DATA]
< else:
< tagged_data = None
---
> tagged_data = attrs.pop(TAGGED_DATA, None)
480,481d460
<
<
487,489c466
< if self == other:
< return True
< return other.extends(self)
---
> return self == other or other.extends(self)
496,498c473,474
< r = {}
< for name in self.__attrs.keys():
< r[name] = 1
---
> r = self.__attrs.copy()
>
500,501c476,477
< for name in base.names(all):
< r[name] = 1
---
> r.update(dict.fromkeys(base.names(all)))
>
513,514c489,490
< for name, d in self.__attrs.items():
< r[name] = d
---
> for base in self.__bases__[::-1]:
> r.update(dict(base.namesAndDescriptions(all)))
516,519c492
< for base in self.__bases__:
< for name, d in base.namesAndDescriptions(all):
< if name not in r:
< r[name] = d
---
> r.update(self.__attrs)
572d544
< pass
586c558,559
< for b in self.__bases__: b.__d(dict)
---
> for b in self.__bases__:
> b.__d(dict)
589,590c562,564
< r = getattr(self, '_v_repr', self)
< if r is self:
---
> try:
> return self._v_repr
> except AttributeError:
597c571
< return r
---
> return r
600,603c574,576
< # TRICK! Create the call method
< #
< # An embedded function is used to allow an optional argument to
< # __call__ without resorting to a global marker.
---
> # Mind the closure. It serves to keep a unique marker around to
> # allow for an optional argument to __call__ without resorting
> # to a global marker.
605,610c578
< # The evility of this trick is a reflection of the underlying
< # evility of "optional" arguments, arguments whose presense or
< # absense changes the behavior of the methods.
< #
< # I think the evil is necessary, and perhaps desireable to
< # provide some consistencey with the PEP 246 adapt method.
---
> # This provides some consistency with the PEP 246 adapt method.
699,702c667,671
< if adapter is None:
< if alternate is not marker:
< return alternate
<
---
> if adapter is not None:
> return adapter
> elif alternate is not marker:
> return alternate
> else:
705,706d673
< return adapter
<
709c676
< __call__ = __call__() # TRICK! Make the *real* __call__ method
---
> __call__ = __call__() # Make the closure the *real* __call__ method.
802d768
<
861c827
< sig = "("
---
> sig = []
863c829
< sig = sig + v
---
> sig.append(v)
865,866c831
< sig = sig + "=%s" % `self.optional[v]`
< sig = sig + ", "
---
> sig[-1] += "=%s" % `self.optional[v]`
868c833
< sig = sig + ("*%s, " % self.varargs)
---
> sig.append("*%s" % self.varargs)
870,874c835
< sig = sig + ("**%s, " % self.kwargs)
<
< # slice off the last comma and space
< if self.positional or self.varargs or self.kwargs:
< sig = sig[:-2]
---
> sig.append("**%s" % self.kwargs)
876,877c837
< sig = sig + ")"
< return sig
---
> return "(%s)" % ", ".join(sig)
896,897c856
< for i in range(len(defaults)):
< opt[names[i+nr]] = defaults[i]
---
> opt.update(dict(zip(names[nr:], defaults)))
More information about the Interface-dev
mailing list