GNOME Bugzilla – Bug 657915
__repr__ are not right.
Last modified: 2018-01-10 20:12:10 UTC
>>> Gtk.WindowType <class 'gi.repository.Gtk.GtkWindowType'> I think this should show <class 'gi.repository.Gtk.WindowType'>
I can't find where we set or output this
Created attachment 196495 [details] repr test program Simple test program in C. Break on line 63 (PyObject_Print(wt, stdout, 0);) and step away. You'll eventually see PyObject_Repr is called in Objects/object.c:305. Stepping further into PyObject_Repr, we pass through Objects/object.c:381 and end up in type_repr from Objects/typeobject.c:674. Here Python helpfully builds a repr string ala "<class...". I would have expected the repr string to be "<enum..." and computed by pyg_enum_repr from gi/_gobject/pygenum.c instead? Also, "from gi.repository import Gtk; print(type(Gtk.WindowType))" prints <type 'type'>. Shouldn't that be gobject.GEnum? Have run out of time to take this further but maybe this info sparks an idea with somebody :) ps Tested with Python-2.7.2, just so we refer to the same line numbers.
Note, bug 639904 is a more specific example for Enums, I attached a sketch patch there for Enums only. But I mentioned that this problem applies to all GI identifiers, so making 639904 a duplicate.
*** Bug 639904 has been marked as a duplicate of this bug. ***
Note that repr() shoudl not show things like "<enum ..>", but a valid Python expression.
I fixed a couple easy ones here: https://git.gnome.org/browse/pygobject/commit/?id=9b02b29016
Gdk.Event (and its enumerations) would be a great candidate for this.
A few more inconsistencies (using 3.16): from gi.repository import Gdk, Gtk, GLib print(repr(Gdk.EventAny), repr(Gdk.EventAny())) print(repr(Gtk.Border), repr(Gtk.Border())) print(repr(Gdk.Keymap), repr(Gdk.Keymap.get_default())) print(repr(Gtk.Button), repr(Gtk.Button())) print(repr(Gdk.DragAction), repr(Gdk.DragAction(3 | 64))) print(repr(Gdk.WindowState), repr(Gdk.WindowState(0))) print(repr(Gtk.Align), repr(Gtk.Align.START)) print(repr(GLib.SpawnFlags), repr(GLib.SpawnFlags.SEARCH_PATH)) <class 'gi.overrides.Gdk.EventAny'> <void at 0x134c060> <class 'gi.repository.Gtk.Border'> <GtkBorder at 0x12956b0> <class 'gi.repository.Gdk.Keymap'> <gtk.gdk.X11Keymap object at 0x7fdc8e4bbea0 (GdkX11Keymap at 0x1355060)> <class 'gi.overrides.Gtk.Button'> <Button object at 0x7fdc8e4bbea0 (GtkButton at 0x1356170)> <class 'gi.repository.Gdk.GdkDragAction'> <flags GDK_ACTION_DEFAULT | GDK_ACTION_COPY of type GdkDragAction> <class 'gi.repository.Gdk.GdkWindowState'> <flags 0 of type GdkWindowState> <class 'gi.repository.Gtk.GtkAlign'> <enum GTK_ALIGN_START of type GtkAlign> <class 'gi.repository.GLib.PyGLibSpawnFlags'> <flags G_SPAWN_SEARCH_PATH of type PyGLibSpawnFlags> Suggested: <class 'gi.overrides.Gdk.EventAny'> <Gdk.EventAny at 0x1ace210 (void at 0x1933690)> # no gtype.. <class 'gi.repository.Gtk.Border'> <Gtk.Border at 0x1ace210 (GtkBorder at 0x1933690)> <class 'gi.repository.Gdk.Keymap'> <GdkX11Keymap object at 0x7f3d53233f30 (GdkX11Keymap at 0x1b8e060)> # GdkX11 not loaded, so unknown except gtype <class 'gi.overrides.Gtk.Button'> <Gtk.Button object at 0x7f3d53233f30 (GtkButton at 0x1b90170)> <class 'gi.repository.Gdk.DragAction'> Gdk.DragAction.DEFAULT | Gdk.DragAction.COPY | Gdk.DragAction(64) <class 'gi.repository.Gdk.WindowState'> Gdk.WindowState(0) <class 'gi.repository.Gtk.Align'> Gtk.Align.START <class 'gi.repository.GLib.SpawnFlags'> GLib.SpawnFlags.SEARCH_PATH any objections?
Created attachment 304996 [details] [review] Improve and unify __repr__ format for PyGObject, PyGBoxed and PyGIStruct This addresses the first half of the above examples: OLD: ("<class 'gi.overrides.Gdk.EventAny'>", '<void at 0x1dcf420>') ("<class 'gi.repository.Gtk.Border'>", '<GtkBorder at 0x1ea7370>') ("<class 'gi.repository.Gdk.Keymap'>", '<gtk.gdk.X11Keymap object at 0x7fc3034758c0 (GdkX11Keymap at 0x1f73030)>') ("<class 'gi.overrides.Gtk.Button'>", '<Button object at 0x7fc3034758c0 (GtkButton at 0x1f75190)>') NEW: ("<class 'gi.overrides.Gdk.EventAny'>", '<Gdk.EventAny object at 0x7fb8c2608470 (void at 0x2a33ab0)>') ("<class 'gi.repository.Gtk.Border'>", '<Gtk.Border object at 0x7fb8c229a4c8 (GtkBorder at 0x2af32a0)>') ("<class 'gi.repository.Gdk.Keymap'>", '<__gi__.GdkX11Keymap object at 0x7fb8ca1a3410 (GdkX11Keymap at 0x2bc70b0)>') ("<class 'gi.overrides.Gtk.Button'>", '<Gtk.Button object at 0x7fb8ca1a3410 (GtkButton at 0x2bc9170)>')
Created attachment 304998 [details] [review] Improve and unify __repr__ format for PyGObject, PyGBoxed and PyGIStruct (and now compiles with Python3.. :/ )
Review of attachment 304998 [details] [review]: This is a good start, just a few comments and alternative ideas I'd like to hear your feedback on. I think I would actually prefer a combination of the brevity of the previous implementation with the more accurate namespace of this. For example: <Gtk.Button object at 0x7fb8ca1a3410 (GtkButton at 0x2bc9170)> would instead be: <Gtk.Button at 0x2bc9170)> The pointer being wrapped seems more interesting than the Python object pointer. Thoughts? ::: gi/pygboxed.c @@ +76,2 @@ + module = PyObject_GetAttrString (self, "__module__"); + if (module == NULL || !PYGLIB_PyUnicode_Check (module)) If module is non-null and PYGLIB_PyUnicode_Check() fails, module will be leaked. ::: gi/pygobject.c @@ +738,3 @@ + /* Something special to point out that it's not accessible through + * gi.repository */ + o = PYGLIB_PyUnicode_FromString ("__gi__"); What about simply using the typename by itself? instead of: <__gi__.GdkX11Keymap object at 0x7fb8ca1a3410 (GdkX11Keymap at 0x2bc70b0)> We'd have: <GdkX11Keymap at 0x2bc70b0> Or do you think it removes useful debugging information? @@ +1103,3 @@ + + module = PyObject_GetAttrString ((PyObject *)self, "__module__"); + if (module == NULL || !PYGLIB_PyUnicode_Check (module)) Same here.
Also note the comment #5 that if possible, repr should return a valid Python expressions [1]. This is what I did here: https://git.gnome.org/browse/pygobject/commit/?id=9b02b29016 https://docs.python.org/2/reference/datamodel.html#object.__repr__
(In reply to Simon Feltman from comment #11) > I think I would actually prefer a combination of the brevity of the previous > implementation with the more accurate namespace of this. For example: > > <Gtk.Button object at 0x7fb8ca1a3410 (GtkButton at 0x2bc9170)> > > would instead be: > > <Gtk.Button at 0x2bc9170)> Actually, it looks like GObjects have always looked like the first example here. I guess I was talking about the changes to structs, but GObjects should follow if you take that direction.
(In reply to Simon Feltman from comment #11) > Review of attachment 304998 [details] [review] [review]: > > This is a good start, just a few comments and alternative ideas I'd like to > hear your feedback on. Thanks. > I think I would actually prefer a combination of the brevity of the previous > implementation with the more accurate namespace of this. For example: > > <Gtk.Button object at 0x7fb8ca1a3410 (GtkButton at 0x2bc9170)> > > would instead be: > > <Gtk.Button at 0x2bc9170)> > > The pointer being wrapped seems more interesting than the Python object > pointer. Thoughts? One use of the wrapper pointer (afaics?) is that for boxed types there can be two wrappers for the same pointer so that "A == B" but "A is not B". The different pointer make that more clear. But I don't mind stripping it (as long as it's consistent across all types) > ::: gi/pygboxed.c > @@ +76,2 @@ > + module = PyObject_GetAttrString (self, "__module__"); > + if (module == NULL || !PYGLIB_PyUnicode_Check (module)) > > If module is non-null and PYGLIB_PyUnicode_Check() fails, module will be > leaked. Thanks > ::: gi/pygobject.c > @@ +738,3 @@ > + /* Something special to point out that it's not accessible through > + * gi.repository */ > + o = PYGLIB_PyUnicode_FromString ("__gi__"); > > What about simply using the typename by itself? instead of: > > <__gi__.GdkX11Keymap object at 0x7fb8ca1a3410 (GdkX11Keymap at > 0x2bc70b0)> > > We'd have: > > <GdkX11Keymap at 0x2bc70b0> > > Or do you think it removes useful debugging information? I wanted to point out that one should not go looking for it in the gir modules and that it's "special" (The previous code used __main__ for things not matching the static binding modules e.g. Gio.File.new_for_path("")) > @@ +1103,3 @@ > + > + module = PyObject_GetAttrString ((PyObject *)self, "__module__"); > + if (module == NULL || !PYGLIB_PyUnicode_Check (module)) > > Same here. Thanks (In reply to Simon Feltman from comment #12) > Also note the comment #5 that if possible, repr should return a valid Python > expressions [1]. This is what I did here: > https://git.gnome.org/browse/pygobject/commit/?id=9b02b29016 > > https://docs.python.org/2/reference/datamodel.html#object.__repr__ Does that apply to this patch in some way? I plan to use valid expressions for enums and flags (see examples in comment #8)
Created attachment 305131 [details] [review] enum/flags: use gir info for type names and __repr__ instead of the gtype name For example __name__ is now SpawnFlags instead of PyGLibSpawnFlags and __repr__ shows GLib.SpawnFlags in stead of PyGLibSpawnFlags. This doesn't improve the repr regarding value names as I wanted to keep the changes minimal. But at least it makes it easier to look things up in the API docs. Bug #682405 should probably be worked on first before continuing here.
Review of attachment 305131 [details] [review]: ::: gi/gimodule.c @@ +204,3 @@ + } + + g_base_info_unref (info); Seems like this chunk and the one from _wrap_pyg_enum_add could be decomposed and parameterized into a common function by passing the either pyg_flags_add or pyg_enum_add? ::: gi/pygenum.c @@ +113,3 @@ + namespace, Py_TYPE (self)->tp_name); + + value = enum_class->values[index].value_name; `module` is also leaked here. ::: gi/pygflags.c @@ +127,3 @@ + namespace += 1; + } + } Looks like `module` is being leaked in the normal case here. @@ +134,3 @@ + retval = g_strdup_printf("<flags %ld of type %s.%s>", + PYGLIB_PyLong_AsUnsignedLong (self), + namespace, Py_TYPE (self)->tp_name); The `module` decref should probably go after 'namespace' is used since it would be a cache on that object.
Created attachment 314125 [details] [review] [v2] Improve and unify __repr__ format for PyGObject, PyGBoxed and PyGIStruct Fixed the two leaks in the error case. Everything else as before. I personally prefer the additional information as stated above, but if I'm alone here I'm happy to remove it.
Created attachment 314126 [details] [review] [v2] enum/flags: use gir info for type names and __repr__ instead of the gtype name Thanks. Fixed the leaks and added the suggested function.
Review of attachment 314125 [details] [review]: This seems fine. Thanks.
Review of attachment 314126 [details] [review]: Nicely done, thanks.
Thanks. https://git.gnome.org/browse/pygobject/commit/?id=6b702c052e9f26e809cff494f0c896d17a514c64 https://git.gnome.org/browse/pygobject/commit/?id=b1788c9a445c8a820121c42260bcbdbc3ae8dfba
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/pygobject/issues/20.