After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 686835 - Remove gi.overrides.overridefunc
Remove gi.overrides.overridefunc
Status: RESOLVED FIXED
Product: pygobject
Classification: Bindings
Component: introspection
unspecified
Other Linux
: Normal minor
: ---
Assigned To: Nobody's working on this now (help wanted and appreciated)
Python bindings maintainers
Depends on:
Blocks:
 
 
Reported: 2012-10-25 03:36 UTC by Simon Feltman
Modified: 2017-03-27 14:57 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Test which raises an exception (do not commit) (1.25 KB, text/plain)
2012-10-25 03:41 UTC, Simon Feltman
  Details
Remove gi.overrides.overridefunc (2.11 KB, patch)
2017-03-21 18:25 UTC, Christoph Reiter (lazka)
committed Details | Review

Description Simon Feltman 2012-10-25 03:36:42 UTC
This seems like unnecessary code which obfuscates things and also clobbers introspection modules version of functions with the overridden function. Overridden functions are accessed through the DynamicModules resolution ordering anyway so this shouldn't be needed.

gi.overrides.overridefunc or gi.overrides.override could be changed to just return the input func instead of wrapping it as this also adds another layer of function calls.
Comment 1 Simon Feltman 2012-10-25 03:41:05 UTC
Created attachment 227216 [details]
Test which raises an exception (do not commit)

This patch shows the overridden function that is assigned into the introspection module is never reached. Using this with make check, the exception is never raised.
Comment 2 Simon Feltman 2012-10-25 04:04:07 UTC
I'm wrong about this. It turns out overridden functions only occur three times in all the current overrides and none of them are tested. Furthermore, they are not added to the __all__ table in overrides which is probably why the clobbering exists in the first place.

Gtk.main_quit
Gtk.stock_lookup
Gdk.color_parse

The system should refrain from clobbering the original method in the introspection module if possible. I will look at adding overridden functions to the registry used by types.
Comment 3 Martin Pitt 2013-03-01 13:07:58 UTC
The only remaining one is now Gtk.main_quit, the other two already disappeared with 
http://git.gnome.org/browse/pygobject/commit/?id=4f6ebcfe060.

http://git.gnome.org/browse/pygobject/commit/?id=1edc4ba adds tests for these three functions, and http://git.gnome.org/browse/pygobject/commit/?id=1dc2bc drops overridefunc.
Comment 4 Simon Feltman 2013-03-01 21:37:00 UTC
So what I was complaining about is we clobber the original introspection modules version of the function, not add it to the override module. So instead of:

Gtk.stock_lookup = strip_boolean_result(Gtk.stock_lookup)

We would have:

stock_lookup = strip_boolean_result(Gtk.stock_lookup)
__all__.append('stock_lookup')

This leaves the original module unaltered and the overrides module as layer on top without side-affects.
Comment 5 Simon Feltman 2013-03-01 21:43:21 UTC
We might consider making sure we don't break compatibility in the case of overrides outside of PyGObject. So if someone is using "override" to decorate a function it shouldn't break, this could be achieved by just returning the original arg if it is a function:

def override(type_):
     '''Decorator for registering an override'''
     if isinstance(type_, types.FunctionType):
         return type_
     ...
Comment 6 Martin Pitt 2013-03-04 16:17:33 UTC
(In reply to comment #4)
> So what I was complaining about is we clobber the original introspection
> modules version of the function, not add it to the override module.

Oh sorry, I see what you mean. Fixed in

http://git.gnome.org/browse/pygobject/commit/?id=25a6f9
Comment 7 Martin Pitt 2013-03-05 11:13:35 UTC
(In reply to comment #5)
> We might consider making sure we don't break compatibility in the case of
> overrides outside of PyGObject. So if someone is using "override" to decorate a
> function it shouldn't break, this could be achieved by just returning the
> original arg if it is a function:
> 
> def override(type_):
>      '''Decorator for registering an override'''
>      if isinstance(type_, types.FunctionType):
>          return type_
>      ...

I tried that, but with that the overridden function is not recognized at all (see e. g. test_overrides_gtk.TestGtk.test_gtk_main). Presumably they need to be added to __all__, but that's not what e. g. the current GEdit plugins do. This caused bug 695199.

So I reverted http://git.gnome.org/browse/pygobject/commit/?id=1dc2bc for now, and reopen this. If we want to simplify this, we need to find a way to do the above but automatically add the newly defined function to __all__ (or do the equivalent).
Comment 8 Christoph Reiter (lazka) 2017-03-21 18:25:08 UTC
Created attachment 348435 [details] [review]
Remove gi.overrides.overridefunc

Move the code into override()

----

With the new override import code it no longer clobbers the introspection module since sys.modules contains a proxy module now. But moving it into override() can't hurt I guess.