PySide Bugzilla Closed for New Bugs

PySide is now a Qt Add-on and uses the Qt Project's JIRA Bug Tracker instead of this Bugzilla instance. This Bugzilla is left for reference purposes.

First Last Prev Next    This bug is not in your last search results.
Bug 593 - __repr__ should be more Pythonic
: __repr__ should be more Pythonic
Status: CLOSED FIXED
Product: PySide
Classification: Unclassified
Component: PySide
: HEAD
: All All
: P3 normal
Assigned To: renato filho
:
:
:
  Show dependency tree / graph
 
Reported: 2011-01-10 13:08 EET by Farsmo
Modified: 2011-02-02 15:46 EET (History)
9 users (show)

See Also:

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Farsmo 2011-01-10 13:08:00 EET 
It's great that __repr__ is showing more information than it used to. Now we
can think about refining exactly what is displayed.

Most importantly, currently repr() always shows the address of the object. I
know that most if not all types are mutable in Qt, but that doesn't mean we
need to display the address: after all, Python's list and dict types are
mutable, yet they are displayed without their address or any other way to
detect aliasing. A good rule of thumb to find out if the address of the object
should be hidden is: are all the parameters describing the internal state
already displayed? Another rule of thumb, which seems to coincide with the
first in most cases, would be: do we have QMyClass(*params)==QMyClass(*params)
for any value of params such that x=QMyClass(*params) satisfies x==x? If we do,
then it makes a lot of sense to also have
repr(QMyClass(*params))==repr(QMyClass(*params)), which is only possible if
repr does not print the address.

So the current shiboken-generated implementation is fine for all classes that
inherit QObject as their state is never printed entirely -- it would entail
printing the tree of all descendants (also, even if it was possible we have
QObject()!=QObject()). A new implementation would only be needed for a few
classes (QPoint[F], QRect[F], QColor, QPen...).

For these classes it would be great to have repr return a true
Python-executable expression. For example,
    >>> QRectF(1,2,3,4)
    <PySide.QtCore.QRectF(1,2 3x4)  at 0x01E6E230>
should be either
    PySide.QtCore.QRectF(1,2, 3,4)
or
    PySide.QtCore.QRectF(1, 2, 3, 4)

Likewise,
    >>> QColor.fromRgb(0,128,255)
    <PySide.QtGui.QColor(ARGB 1, 0, 0.501961, 1)  at 0x01E6E260>
should be
    PySide.QtGui.QColor.fromRgbF(0, 0.501961, 1)
or
    PySide.QtGui.QColor.fromRgbF(0, 0.501961, 1, 1)

Finally there is the issue of floating-point precision: QDebug cares about
giving a representation "good enough for debugging" so it truncates
floating-point values to a reasonable precision. This is good for __str__, but
for __repr__ we probably want to display the exact value, like float.__repr__
does. One notable exception is QColor, where truncation is fine as 6 digits are
always enough to recover the 16-bit internal state.
Comment 1 Hugo Parente Lima 2011-01-10 14:28:45 EET 
__repr__ is used for debugging purporses, so IMO there's no reason to hide
memory address of any other information that could help the programmer to debug
their app.

I vote for "WONTFIX"..
Comment 2 Farsmo 2011-01-10 23:21:54 EET 
Well is there any good reason why the programmer would need that address
information?
I scratched my head and I can only see three really realistic use cases:
1. he wants to debug PySide/Shiboken itself
2. he wants to obtain aliasing information (i.e. distinguish between [a,a] and
[a,b] when a==b)
3. he wants to examine memory locality

Other than that, I could only come up with crazy low-level debugging scenarios
like:
- he wants the address of the QPoint to look up the corresponding memory zone
manually when debugging (but since he already has access to a high-level dump
of the full content, this makes no sense unless he's debugging Qt or PySide
itself)
- he wants the address of the QPoint to manually match an address printed with
Python with an address in a debugger of a low-level language that doesn't have
an easy way to dump the contents of a QPoint, in order to find out those
contents
These hardly make any sense, and even if it did make sense it would be very
easy for the programmer to explicitly request the address.


So let's go back to our three use cases:

1. This use case makes sense because if the bridge between low-level and
high-level representation is broken, the high-level representation given by
repr() might not be enough. But only PySide developers themselves are likely to
ever find themselves in this situation, so a PySide compile-time debugging flag
to disable all custom __repr__ should be enough. Including it for the release
version would be the same as a standard Python dict that would always print out
debugging information just in case there was a bug in the dict implementation:
only CPython developers might ever need to do that.

2. This is something that *can* happen, but there is no reason for it to be
more important to provide aliasing information about QPoints than about lists
or dicts. Python operates under the design assumption that it is OK to abstract
away the aliasing information in order to provide a compact, readable and
possibly executable representation to the user, so it makes sense to follow
that lead: after all, the damage has already been done as aliasing information
from lists is stripped by Python (and even by PySide, since QList is converted
implicitly to list), so the user still has to be aware of that danger.

3. This is a bit like the previous case: it *could* be useful, but Python
doesn't provide it for its native types, so why would it matter for a QPoint
but not for a float or a string? After all, floats or ints are more likely to
be performance-critical than QPoints.


So, my opinion on this matter: the address printed by __repr__ is unlikely to
be very useful to most developers, and cluttering __repr__ with rarely-useful
information will hinder debugging much more than it will help. Not only is it a
good thing in itself, but it's also more consistent with the way Python does
things; a core design goal of a good Python binding should be to do things the
Python way.


Compare the readability:

[[<PySide.QtCore.QPoint(0,0)  at 0x01CBE200>, <PySide.QtCore.QPoint(0,0)  a
t 0x01CBE230>, <PySide.QtCore.QPoint(0,0)  at 0x01CBE1B8>, <PySide.QtCore.Q
Point(0,0)  at 0x01CBE248>, <PySide.QtCore.QPoint(0,0)  at 0x01CBE278>, <Py
Side.QtCore.QPoint(0,0)  at 0x01CBE290>, <PySide.QtCore.QPoint(0,0)  at 0x0
1CBE2A8>, <PySide.QtCore.QPoint(0,0)  at 0x01CBE140>, <PySide.QtCore.QPoint
(0,0)  at 0x01CBE158>, <PySide.QtCore.QPoint(0,0)  at 0x01CBE170>, <PySide.
QtCore.QPoint(0,0)  at 0x01CBE188>, <PySide.QtCore.QPoint(0,0)  at 0x01CBE1
A0>], [<PySide.QtCore.QPoint(0,0)  at 0x01CBE200>, <PySide.QtCore.QPoint(0,
0)  at 0x01CBE230>, <PySide.QtCore.QPoint(0,0)  at 0x01CBE1B8>, <PySide.QtC
ore.QPoint(0,0)  at 0x01CBE248>, <PySide.QtCore.QPoint(0,0)  at 0x01CBE278>
, <PySide.QtCore.QPoint(0,0)  at 0x01CBE290>, <PySide.QtCore.QPoint(0,0)  a
t 0x01CBE2A8>, <PySide.QtCore.QPoint(0,0)  at 0x01CBE140>, <PySide.QtCore.Q
Point(4,5)  at 0x01CBE128>, <PySide.QtCore.QPoint(0,0)  at 0x01CBE170>, <Py
Side.QtCore.QPoint(0,0)  at 0x01CBE188>, <PySide.QtCore.QPoint(0,0)  at 0x0
1CBE1A0>, <PySide.QtCore.QPoint(0,0)  at 0x01CBE200>, <PySide.QtCore.QPoint
(0,0)  at 0x01CBE230>, <PySide.QtCore.QPoint(0,0)  at 0x01CBE1B8>], [<PySid
e.QtCore.QPoint(0,0)  at 0x01CBE200>, <PySide.QtCore.QPoint(0,0)  at 0x01CB
E230>, <PySide.QtCore.QPoint(0,0)  at 0x01CBE1B8>, <PySide.QtCore.QPoint(0,
0)  at 0x01CBE248>, <PySide.QtCore.QPoint(0,0)  at 0x01CBE278>, <PySide.QtC
ore.QPoint(0,0)  at 0x01CBE290>, <PySide.QtCore.QPoint(0,0)  at 0x01CBE2A8>
, <PySide.QtCore.QPoint(0,0)  at 0x01CBE140>, <PySide.QtCore.QPoint(0,0)  a
t 0x01CBE158>, <PySide.QtCore.QPoint(0,0)  at 0x01CBE170>, <PySide.QtCore.Q
Point(0,0)  at 0x01CBE188>, <PySide.QtCore.QPoint(0,0)  at 0x01CBE1A0>]]

[[PySide.QtCore.QPoint(0,0), PySide.QtCore.QPoint(0,0), PySide.QtCore.QPoin
t(0,0), PySide.QtCore.QPoint(0,0), PySide.QtCore.QPoint(0,0), PySide.QtCore
.QPoint(0,0), PySide.QtCore.QPoint(0,0), PySide.QtCore.QPoint(0,0), PySide.
QtCore.QPoint(0,0), PySide.QtCore.QPoint(0,0), PySide.QtCore.QPoint(0,0), P
ySide.QtCore.QPoint(0,0)], [PySide.QtCore.QPoint(0,0), PySide.QtCore.QPoint
(0,0), PySide.QtCore.QPoint(0,0), PySide.QtCore.QPoint(0,0), PySide.QtCore.
QPoint(0,0), PySide.QtCore.QPoint(0,0), PySide.QtCore.QPoint(0,0), PySide.Q
tCore.QPoint(0,0), PySide.QtCore.QPoint(4,5), PySide.QtCore.QPoint(0,0), Py
Side.QtCore.QPoint(0,0), PySide.QtCore.QPoint(0,0), PySide.QtCore.QPoint(0,
0), PySide.QtCore.QPoint(0,0), PySide.QtCore.QPoint(0,0)], [PySide.QtCore.Q
Point(0,0), PySide.QtCore.QPoint(0,0), PySide.QtCore.QPoint(0,0), PySide.Qt
Core.QPoint(0,0), PySide.QtCore.QPoint(0,0), PySide.QtCore.QPoint(0,0), PyS
ide.QtCore.QPoint(0,0), PySide.QtCore.QPoint(0,0), PySide.QtCore.QPoint(0,0
), PySide.QtCore.QPoint(0,0), PySide.QtCore.QPoint(0,0), PySide.QtCore.QPoi
nt(0,0)]]
Comment 3 Matti Airas 2011-01-11 08:19:06 EET 
Thanks for the bug report. I'm actually siding here with Farsmo: while the most
common use case for __repr__ probably indeed is debugging, returning the object
addresses is hardly useful for Python programmers, and since the __repr__
output tends to be already quite verbose in real-life use cases, cluttering it
further is just unnecessary burden.

Also, I've found it useful in some of my personal (non-PySide, admittedly)
projects to be able to feed the __repr__ output back to the Python interpreted.
If the output is not parseable Python expression, this of course is not
possible. So, in my opinion, PySide should conform to [1] in its __repr__
output: valid Python expressions, and if that's not possible (as is the case
with PySide very often), a string of the form "<...object description...>".

[1] http://docs.python.org/reference/datamodel.html#object.__repr__

If the address is desired, in CPython (which is our frame of reference, after
all) one can always use id() to provide that.

I'm prioritizing the bug P3 for now, and asking comments on the mailing list.
Comment 4 Farsmo 2011-01-11 14:00:11 EET 
Glad to hear that :)

Now that I've got my foot in the door, might I suggest adding an option to drop
the "PySide.QtCore/QtGui." prefix to make the output less verbose? Of course it
might not be sensible to do that by default, but if your project uses the
programming convention "from PySide.QtFoo import *" then there is no point in
printing an obnoxious prefix when debugging.

Don't forget that the prefix will show up more often if we make all __repr__
executable, as QDebug doesn't show nested prefixes:

(current PySide)
>>> QPen()
<PySide.QtGui.QPen(0,QBrush(QColor(ARGB 1, 0, 0, 0) , SolidPattern )  ,
SolidLin
e , 16 , 64 , QVector() , 0 , 2 )   at 0x01E6E290>

(executable fully-qualified code)
PySide.QtGui.QPen(0, PySide.QtGui.QBrush(PySide.QtGui.QColor.fromRgbF(0, 0, 0,
1), PySide.QtCore.Qt.SolidPattern), PySide.QtCore.Qt.SolidLine, 16, 64, [], 0,
2)

(executable unqualified code)
QPen(0, QBrush(QColor.fromRgbF(0, 0, 0, 1), Qt.SolidPattern), Qt.SolidLine, 16,
64, [], 0, 2)

Obviously it may also be wise to use default arguments to display only
"QPen()", but that's not my point.
Comment 5 Thomas Perl 2011-01-11 14:12:20 EET 
(In reply to comment #4)
> if your project uses the
> programming convention "from PySide.QtFoo import *" then there is no point in
> printing an obnoxious prefix when debugging.
> 
> Don't forget that the prefix will show up more often if we make all __repr__
> executable, as QDebug doesn't show nested prefixes:

+1

> Obviously it may also be wise to use default arguments to display only
> "QPen()", but that's not my point.

No, I think it's a good idea that the parameters are shown even if they are the
default arguments - makes it easier to verify the assumption that a given
instance has specific properties.
Comment 6 Hugo Parente Lima 2011-01-11 14:54:27 EET 
Our current __repr__ output is basically what you get when you do: "qDebug() <<
t;"

If you plan to do a more elaborate rule for what __repr__ should return it
means hand write the __repr__ code for *a lot* of classes, IMO it's just not
feasible due to the time we'll need to spend to do this boring task.

So I think is better to think in a automatic way of creating a usefull __repr__
implementation or hire more monkeys to hand write the __repr__ function for
each Qt class :-)

If you guys want to do some experiments, the code that generates the __repr__
function is located at <shiboken>/generator/cppgenerator.cpp line 3891 and it's
get called at line 2443.
Comment 7 Farsmo 2011-01-11 16:05:48 EET 
Like I said on the mailing list __reduce__ is a good starting point, it just
needs a little metadata.


> No, I think it's a good idea that the parameters are shown even if they are the
default arguments - makes it easier to verify the assumption that a given
instance has specific properties.

I assume it depends on how obvious such properties would be. It's true that for
QPen even the list of properties is far from obvious. But for example,
QColor.fromRgbF(r, g, b) would be preferable to QColor.fromRgbF(r, g, b, 1) as
the alpha value of 1 is both obvious and irrelevant for many purposes.
Comment 8 Matti Airas 2011-01-12 06:29:16 EET 
(In reply to comment #4)
> Glad to hear that :)
> 
> Now that I've got my foot in the door, might I suggest adding an option to drop
> the "PySide.QtCore/QtGui." prefix to make the output less verbose? Of course it
> might not be sensible to do that by default, but if your project uses the
> programming convention "from PySide.QtFoo import *" then there is no point in
> printing an obnoxious prefix when debugging.

IMO, this is somewhat a separate issue and therefore there should be a separate
bug about it.

As for the current issue, I agree with Hugo that the fix should be creating
(fixing) a sensible automatic __repr__ generation -- tweaking the __repr__
output of individual classes would require a totally unmanageable amount of
effort.
Comment 9 Farsmo 2011-01-13 15:56:51 EET 
Actually because of bug 611 I wasn't aware that enum information was being
kept, so there really shouldn't be any metadata to store. Even if we want to be
fussy, QColor would be the only exception.

On the other hand there is a number of types that can't be pickled because they
might depend on a QPixmap (even if they don't in most cases). I'm thinking of
QBrush, on which QPen depends. Should we assume that it's a bug that they can't
be pickled when they don't happen to depend on a QPixmap?


Finally, there is the issue of the __repr__ of enums: if we want executable
code, then repr(PySide.QtCore.Qt.green) should be "PySide.QtCore.Qt.green", not
"<enum-item GlobalColor.green (8)>". On the downside this doesn't give the name
of the enum or the int value, but if needed these would be easy to get with
type() and int(). The upside is that

PySide.QtGui.QPen(0, PySide.QtGui.QBrush(PySide.QtGui.QColor.fromRgbF(0, 0, 0,
1), PySide.QtCore.Qt.SolidPattern), PySide.QtCore.Qt.SolidLine, 16, 64, [], 0,
2)

is much better than

PySide.QtGui.QPen(0, PySide.QtGui.QBrush(PySide.QtGui.QColor.fromRgbF(0, 0, 0,
1), <enum-item BrushStyle.SolidPattern (1)>), <enum-item PenStyle.SolidLine
(1)>, 16, 64, [], 0, 2)

as it can be copy-pasted into code directly.
Comment 10 renato filho 2011-01-27 17:36:17 EET 
fixed on pyside commit:

commit 1a65a287fbf6b9ddbd7d689e4878426a34d837cd
Author: Renato Araujo Oliveira Filho <renato.filho@openbossa.org>
Date:   Wed Jan 26 15:21:37 2011 -0300
Comment 11 Farsmo 2011-01-28 15:11:38 EET 
Great! But enums still have the same repr(): should I open a new bug, or extend
bug 617?
Comment 12 Matti Airas 2011-01-31 05:27:48 EET 
(In reply to comment #11)
> Great! But enums still have the same repr(): should I open a new bug, or extend
> bug 617?

Since the original issue in bug 617 was fixed, please file a new one. It's much
easier for us if the bugs are kept "atomic", with just one issue per bug.
Comment 13 renato filho 2011-01-31 09:47:46 EET 
the bug #617 was fixed,  and I think all problems with __repr__ can you check
that? If any problems with __repr__ still exists could you open a new bug? 


Thanks
Comment 14 Farsmo 2011-01-31 12:35:58 EET 
Bug 654 filed.
Comment 15 renato filho 2011-02-02 15:46:07 EET 
released on beta 5
First Last Prev Next    This bug is not in your last search results.