Created attachment 148387 [details] [review]
Remove trailing whitespace

Created attachment 148388 [details] [review]
Add a new decorators file

With one decorator so far, used to override classes/methods

Created attachment 148389 [details] [review]
Do not always override the __new__ methods

If it's implemented in a subclass, let the subclass __new__
class method be called instead

Created attachment 148390 [details] [review]
Override Gdk.Rectangle constructor and __repr__

Increases PyGTK compatibility by allow keyword
arguments to the constructor and adds a nicer __repr__

Created attachment 148391 [details] [review]
Pythonify. Avoid ; and () around if statements

Review of attachment 148387 [details] [review]:

OK.

Review of attachment 148391 [details] [review]:

OK.

Review of attachment 148388 [details] [review]:

::: gi/decorators.py
@@ +4,3 @@
+# Copyright (C) 2009 Johan Dahlin <johan@gnome.org>
+#
+#   decorators.py: decoratores used in PyGI

typo

Review of attachment 148390 [details] [review]:

Thanks!

Can we imagine a more elegant way to define overrides? (with a metaclass maybe?)

Created attachment 148491 [details] [review]
Another idea (draft)

Here is another idea. I put a ModuleProxy instance in front of the DynamicModule instance and the OverridesModule instance, so that the OverridesModule can actually be a real module, and "import" the DynamicModule instance.

There are still a couple of issues to be resolved (not particular to that implementation):
 - when one overrides a registered wrapper, the overrides type must be registered instead. I added the override decorator for that; it needs a support in C (and could be completely implemented in C actually)
 - in metaclasses, the comparison of the names to avoid doing the initialization stuff on the subclass doesn't work when the subclass has the same name. This must be changed to allow overriding.

And probably some other subtle issues...

Created attachment 148624 [details] [review]
Restore the overrides modules

A complete implementation.

Review of attachment 148389 [details] [review]:

This has been fixed by bug 603036.

Review of attachment 148390 [details] [review]:

Waiting for discussion about the other implementation.

Review of attachment 148388 [details] [review]:

Ditto.

Review of attachment 148624 [details] [review]:

I'm fine with it but would be nice if Johan gave it a look and said if it matches his needs. Though I would like to see this committed soonish because I have work to do with overrides.

Review of attachment 148624 [details] [review]:

::: gi/module.py
@@ -77,3 +77,3 @@
 class DynamicModule(object):
-        path = repository.get_typelib_path(self.__namespace__)
-    def __str__(self):
+        self.__namespace__ = namespace
+    def __init__(self, namespace):

Excessive use of _

@@ -86,3 +82,3 @@
         if not info:
         info = repository.find_by_name(self.__namespace__, name)
-            raise AttributeError("%r object has no attribute %r" % (
+            raise AttributeError

Could use a better exception string

@@ -161,6 +156,9 @@
-    def __members__(self):
-        r = []
-        for type_info in repository.get_infos(self.__namespace__):
... 3 more ...
+        self.__name__ = name
+        return "<DynamicModule %r from %r>" % (self.__namespace__, path)
+
... 6 more ...

Why the excessive use of _ ?

@@ -161,6 +156,15 @@
-    @property
-        return r
-    def __members__(self):
... 3 more ...
+        return "<DynamicModule %r from %r>" % (self.__namespace__, path)
+        path = repository.get_typelib_path(self.__namespace__)
+
... 12 more ...

Will the ModuleProxy ever proxy more than two modules?

attr = getattr(self._proxy_module, name, None)
if attr is None:
   attr = getattr(self._generated_module, name, None)

if attr is None:
   raise AttributeError(..)
return attr

@@ -161,6 +156,16 @@
-        for type_info in repository.get_infos(self.__namespace__):
-        return r
-            r.append(type_info.get_name())
... 3 more ...
+        path = repository.get_typelib_path(self.__namespace__)
+
+    def __repr__(self):
... 13 more ...

You shouldn't call hasattr and then getattr, better to just do one getattr call and pass in None as the default argument.

@@ -161,6 +156,18 @@
-        r = []
-            r.append(type_info.get_name())
-        return r
... 3 more ...
+        self.__name__ = name
+
+        path = repository.get_typelib_path(self.__namespace__)
... 15 more ...

Ditto

@@ -161,6 +156,27 @@
-        r = []
-            r.append(type_info.get_name())
-        return r
... 3 more ...
+class ModuleProxy(object):
+
+        path = repository.get_typelib_path(self.__namespace__)
... 24 more ...

There's no need for this, python will update __dict__ for you after a successful call to __getattr__

::: gi/overrides/Gdk.py
@@ -6,1 +16,1 @@
 

Can you please implement __repr__ here as well, as I did in my patch?
And add a test case that it actually works.

@@ +18,1 @@
+__export__ = [Rectangle]

Perhaps __overridden__ = [] or even __all__ = [] ?

Review of attachment 148624 [details] [review]:

::: gi/module.py
@@ -77,3 +77,3 @@
 class DynamicModule(object):
-        path = repository.get_typelib_path(self.__namespace__)
-    def __str__(self):
+        self.__namespace__ = namespace
+    def __init__(self, namespace):

Excessive use of _

@@ -86,3 +82,3 @@
         if not info:
         info = repository.find_by_name(self.__namespace__, name)
-            raise AttributeError("%r object has no attribute %r" % (
+            raise AttributeError

Could use a better exception string

@@ -161,6 +156,9 @@
-    def __members__(self):
-        r = []
-        for type_info in repository.get_infos(self.__namespace__):
... 3 more ...
+        self.__name__ = name
+        return "<DynamicModule %r from %r>" % (self.__namespace__, path)
+
... 6 more ...

Why the excessive use of _ ?

@@ -161,6 +156,15 @@
-        r = []
-            r.append(type_info.get_name())
-        return r
... 3 more ...
+    def __init__(self, name, namespace, modules):
+
+        path = repository.get_typelib_path(self.__namespace__)
... 12 more ...

Will the ModuleProxy ever proxy more than two modules?

attr = getattr(self._proxy_module, name, None)
if attr is None:
   attr = getattr(self._generated_module, name, None)

if attr is None:
   raise AttributeError(..)
return attr

@@ -161,6 +156,16 @@
-    @property
-        return r
-    def __members__(self):
... 3 more ...
+        return "<DynamicModule %r from %r>" % (self.__namespace__, path)
+        path = repository.get_typelib_path(self.__namespace__)
+
... 13 more ...

You shouldn't call hasattr and then getattr, better to just do one getattr call and pass in None as the default argument.

@@ -161,6 +156,18 @@
-        r = []
-        return r
-            r.append(type_info.get_name())
... 3 more ...
+
+
+        path = repository.get_typelib_path(self.__namespace__)
... 15 more ...

Ditto

@@ -161,6 +156,27 @@
-        r = []
-            r.append(type_info.get_name())
-        return r
... 3 more ...
+                    attribute = None
+
+        path = repository.get_typelib_path(self.__namespace__)
... 24 more ...

There's no need for this, python will update __dict__ for you after a successful call to __getattr__

::: gi/overrides/Gdk.py
@@ -6,1 +16,1 @@
 

Can you please implement __repr__ here as well, as I did in my patch?
And add a test case that it actually works.

@@ +18,1 @@
+__export__ = [Rectangle]

Perhaps __overridden__ = [] or even __all__ = [] ?

Created attachment 148865 [details] [review]
Restore the overrides modules

Created attachment 150859 [details] [review]
Refactore the overrides modules, with tests

Review of attachment 150859 [details] [review]:

Committed as eaf7cb8e.

Damn, I forgot to add gi/overrides/TestGI.py to the patch. Need to fix that.

Fixed in commit 5106523a.