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 617153 - Dont complain if another base has implemented the method
Dont complain if another base has implemented the method
Status: RESOLVED FIXED
Product: pygi
Classification: Deprecated
Component: general
unspecified
Other All
: Normal normal
: 0.6
Assigned To: pygi-maint
pygi-maint
Depends on:
Blocks: 619604
 
 
Reported: 2010-04-29 08:56 UTC by Tomeu Vizoso
Modified: 2010-05-25 14:48 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Dont complain if another base has implemented the method (1.13 KB, patch)
2010-04-29 08:56 UTC, Tomeu Vizoso
committed Details | Review

Description Tomeu Vizoso 2010-04-29 08:56:55 UTC
A proper test would be a pain to add, as we would need to add
a C class implementing the interface, then inheriting from it in
test_gi.py, I'm not sure it's worth it.
Comment 1 Tomeu Vizoso 2010-04-29 08:56:57 UTC
Created attachment 159852 [details] [review]
Dont complain if another base has implemented the method
Comment 2 Abderrahim Kitouni 2010-05-20 20:36:43 UTC
I'm sorry to say that 1) your patch doesn't completely solve the problem, and 2) it doesn't make sense to me. 

From what I see, the patch skips reporting the error if the vfunc has a wrapper function: for GLocalFile (the main use case here :-)) it doesn't complain about dup like it does without the patch but complains about hash (g_file_hash takes a gconstpointer parameter and is thus not considered a method by gi).

A solution to this is to actually bind the do_* functions (I think it's done in pygtk), this is AFAICS the only way we can allow chaining up. (this will probably need some C code)

btw, even after being ironed out, I still don't like the current approach for implementing vfuncs. (and the ugly workarounds it generated like 5e20c018ae and 1d9c6b6d76). What's wrong with my approach in https://bugzilla.gnome.org/show_bug.cgi?id=602736#c9 except that its incomplete? it's actually the pygobject way of doing things.

as for testing, I guess it's trivial to do
>>> from gi.repository import Gio
>>> Gio.file_new_for_path('/home')
and it may be even possible to include this in the tests since gio is part of glib.


On a completely unrelated note, an "abstract" method should be checked based on the vfunc flags rather than belonging to an interface. (although not widely used, gobject (and even vala) allow non-abstract virtual methods in interfaces). I'm not sure it's supported in g-ir-scanner, but it should be the way.
Comment 3 Tomeu Vizoso 2010-05-22 16:58:14 UTC
(In reply to comment #2)
> I'm sorry to say that 1) your patch doesn't completely solve the problem, and
> 2) it doesn't make sense to me. 
> 
> From what I see, the patch skips reporting the error if the vfunc has a wrapper
> function: for GLocalFile (the main use case here :-)) it doesn't complain about
> dup like it does without the patch but complains about hash (g_file_hash takes
> a gconstpointer parameter and is thus not considered a method by gi).
> 
> A solution to this is to actually bind the do_* functions (I think it's done in
> pygtk), this is AFAICS the only way we can allow chaining up. (this will
> probably need some C code)

What do you mean by "actually bind the do_* functions"?

> btw, even after being ironed out, I still don't like the current approach for
> implementing vfuncs. (and the ugly workarounds it generated like 5e20c018ae and
> 1d9c6b6d76). What's wrong with my approach in
> https://bugzilla.gnome.org/show_bug.cgi?id=602736#c9 except that its
> incomplete? it's actually the pygobject way of doing things.

Basically, I think it brings quite a bit of value to our users to check these abnormal conditions at type registration time instead of later at runtime.

Resolving the implementation at runtime is kind of giving up on this issue.

> as for testing, I guess it's trivial to do
> >>> from gi.repository import Gio
> >>> Gio.file_new_for_path('/home')
> and it may be even possible to include this in the tests since gio is part of
> glib.
> 
> 
> On a completely unrelated note, an "abstract" method should be checked based on
> the vfunc flags rather than belonging to an interface. (although not widely
> used, gobject (and even vala) allow non-abstract virtual methods in
> interfaces). I'm not sure it's supported in g-ir-scanner, but it should be the
> way.

Point taken, thanks a lot for taking the time to comment.
Comment 4 Abderrahim Kitouni 2010-05-24 20:52:22 UTC
(In reply to comment #3)
> > A solution to this is to actually bind the do_* functions (I think it's done in
> > pygtk), this is AFAICS the only way we can allow chaining up. (this will
> > probably need some C code)
> 
> What do you mean by "actually bind the do_* functions"?
I mean to have do_* functions that call the given class' implementation rather than dispatching to some overriding method. This will allow to chain up to parent method when overriding, and fix this bug  in the process (we could continue to test for the do_* methods and they will be implemented for base classes). The downside to this approach is that it's far from trivial, and will require to port to GFunctionInvoker, I'm trying to do it, will create a bug when I have something working.
 
> > btw, even after being ironed out, I still don't like the current approach for
> > implementing vfuncs. (and the ugly workarounds it generated like 5e20c018ae and
> > 1d9c6b6d76). What's wrong with my approach in
> > https://bugzilla.gnome.org/show_bug.cgi?id=602736#c9 except that its
> > incomplete? it's actually the pygobject way of doing things.
> 
> Basically, I think it brings quite a bit of value to our users to check these
> abnormal conditions at type registration time instead of later at runtime.
> 
> Resolving the implementation at runtime is kind of giving up on this issue.
I'm not sure what you mean by "at runtime", but it seems to do the right thing, i.e. raise an exception if the class doesn't implement the method
Comment 5 Tomeu Vizoso 2010-05-25 08:35:12 UTC
(In reply to comment #4)
> (In reply to comment #3)
> > > A solution to this is to actually bind the do_* functions (I think it's done in
> > > pygtk), this is AFAICS the only way we can allow chaining up. (this will
> > > probably need some C code)
> > 
> > What do you mean by "actually bind the do_* functions"?
> I mean to have do_* functions that call the given class' implementation rather
> than dispatching to some overriding method. This will allow to chain up to
> parent method when overriding, and fix this bug  in the process (we could
> continue to test for the do_* methods and they will be implemented for base
> classes). The downside to this approach is that it's far from trivial, and will
> require to port to GFunctionInvoker, I'm trying to do it, will create a bug
> when I have something working.

Sorry, but cannot see what you are referring to. My googling couldn't find any reference to GFunctionInvoker other than in this bug, was it a typo?

> > > btw, even after being ironed out, I still don't like the current approach for
> > > implementing vfuncs. (and the ugly workarounds it generated like 5e20c018ae and
> > > 1d9c6b6d76). What's wrong with my approach in
> > > https://bugzilla.gnome.org/show_bug.cgi?id=602736#c9 except that its
> > > incomplete? it's actually the pygobject way of doing things.
> > 
> > Basically, I think it brings quite a bit of value to our users to check these
> > abnormal conditions at type registration time instead of later at runtime.
> > 
> > Resolving the implementation at runtime is kind of giving up on this issue.
> I'm not sure what you mean by "at runtime", but it seems to do the right thing,
> i.e. raise an exception if the class doesn't implement the method

So, if we bind the python bindings to the vfuncs when the type is created, we can warn about all these issues at that moment (typically when the app starts up).

If we tried to resolve the python implementation just before it's called, we'd warn about typos, missing methods, etc. only when that code path is exercised.

I just happen to think that earlier warnings have a big value to developers, but if you can convince the other PyGI developers, I'm ok with stop trying that.

Have filed a bug for it so this discussion doesn't stop other fixes from coming in: https://bugzilla.gnome.org/show_bug.cgi?id=619581
Comment 6 Abderrahim Kitouni 2010-05-25 11:26:35 UTC
> > > What do you mean by "actually bind the do_* functions"?
> > I mean to have do_* functions that call the given class' implementation rather
> > than dispatching to some overriding method. This will allow to chain up to
> > parent method when overriding, and fix this bug  in the process (we could
> > continue to test for the do_* methods and they will be implemented for base
> > classes). The downside to this approach is that it's far from trivial, and will
> > require to port to GFunctionInvoker, I'm trying to do it, will create a bug
> > when I have something working.
> 
> Sorry, but cannot see what you are referring to. My googling couldn't find any
> reference to GFunctionInvoker other than in this bug, was it a typo?

Right, it's GIFunctionInvoker, see for example bug 604074 and bug 604076 (for gjs).
(basically, it's just caching the function pointer and ffi_cif and invoking directly through ffi)
Comment 7 Tomeu Vizoso 2010-05-25 12:23:01 UTC
(In reply to comment #2)
> 
> as for testing, I guess it's trivial to do
> >>> from gi.repository import Gio
> >>> Gio.file_new_for_path('/home')
> and it may be even possible to include this in the tests since gio is part of
> glib.

Looks like you found an unrelated bug, see https://bugzilla.gnome.org/show_bug.cgi?id=619604
Comment 8 Tomeu Vizoso 2010-05-25 12:34:56 UTC
(In reply to comment #3)
> (In reply to comment #2)
> > On a completely unrelated note, an "abstract" method should be checked based on
> > the vfunc flags rather than belonging to an interface. (although not widely
> > used, gobject (and even vala) allow non-abstract virtual methods in
> > interfaces). I'm not sure it's supported in g-ir-scanner, but it should be the
> > way.
> 
> Point taken, thanks a lot for taking the time to comment.

Tracked at https://bugzilla.gnome.org/show_bug.cgi?id=619606
Comment 9 johnp 2010-05-25 14:44:58 UTC
Review of attachment 159852 [details] [review]:

Looks good but can we test this?  If not it looks fine to commit
Comment 10 Tomeu Vizoso 2010-05-25 14:48:00 UTC
Attachment 159852 [details] pushed as 686e10f - Dont complain if another base has implemented the method