EtcdResult.__repr__ should not include __dict__ (imho)

[gst]

Hi,

I just started trying etcd..

I'm suprised to see that the repr of an EtcdResult include the repr()/str() of its whole dict, which is rather counter-producive when you evaluate/test the library in an interactive/debug session.. especially if the returned EtcdResult was one with recursive=True..

Would you mind changing the repr to not anymore include the whole instance dict but only subparts of it (if/when necessary) ?? at least filter out the _children attribute from the repr() would already be a good shot..

[lechat]

I think that __repr__ here is exactly what it supposed to be.

Please read this: http://stackoverflow.com/questions/1436703/difference-between-str-and-repr-in-python

[gst]

@lechat :

I think you miss my point.. my problem is that I've got an EtcdResult, with childrens nodes/leaves also fetched (recurisve=True on the read() call) and its repr() is so long that it makes it practically almost totally useless..

I checked the link, and if you read entirely the best noted answer (http://stackoverflow.com/a/2626364/2451329), while effectively says that "%r" % self.dict can be a good solution at first approach, he also clearly and directly notes that it's dangerous (already possible recursion problems), and can be more or less useless (I extrapolate a bit what he's saying).

He effectively notes that a repr() should be unambiguous but that doesn't mean you have to include the entire dict of any instance object.. which, if one of the attribute stored in it is a list/tuple/dict/any kind of container with possibly unlimited number of elements will lead to the problem I'm describing here.

May I propose something like this :

class EtcdResult(object):
    # ...
    def __repr__(self):
        return "%s(%r)" % (self.__class__, {k:v for k, v in self.__dict__.items() if k != '_children'})

with this repr I have in my case :

$ python -c "import etcd; c = etcd.Client(); res = c.read('/', recursive=True); print(len(repr(res))) ; print(repr(res))"
227
<class 'etcd.EtcdResult'>({'newKey': False, 'etcd_index': 9225476, 'createdIndex': None, 'modifiedIndex': None, 'value': None, 'ttl': None, 'key': None, 'dir': True, 'raft_index': 11651452, 'action': 'get', 'expiration': None})
$

while with the current repr() I have :

$ python -c "import etcd; c = etcd.Client(); res = c.read('/', recursive=True); print(len(repr(res)))"
428680
$

428680 characters.. quite long representation, isn't it ? For the sanity of this comment I don't include its representation so.. ;)

Actually, to be totally unambiguous you could also include the id() of the instance in its repr().. but that's not necessarily required imho.

Eventually you could also include in the repr() the number of elements in the _children attribute (if it's defined / present).

What do you think ?

[gst]

Here is what I finally use temporarily on my side to not be bothered by the problem :

    def __repr__(self): 
        d = self.__dict__.copy()
        d.pop('_children')
        d['nbr_childrens'] = len(getattr(self, '_children', []))
        return "%s(%r)" % (self.__class__, d)     

it could still be enhanced to not make a copy of the dict eventually but well..

[lavagetto]

@gst I kinda concur with @lechat but let me think about it a bit, as I see your point about clarity.

[gst]

thx. repr() being mostly used for/during interactive/debug sessions (and also eventually in logs) so it would be particularly usefull to consider to have this one changed so to not include the entire list of childrens nodes..

[lavagetto]

@gst one thing I don't understand is why you do a read with recursive=True on a large tree and then expect the repr of that to be small.... that's not going to happen of course.

My point is that with your proposal

    r = c.read('/')
    r1 = c.read('/', recursive=True)
    assert r1.__repr__() == r.__repr__()

which is something I definitely don't like.

[gst]

@lavagetto

@gst one thing I don't understand is why you do a read with recursive=True on a large tree and then expect the repr of that to be small.... that's not going to happen of course.

I expect the repr() of anything (unless very special case) to be at least visually easily readable and/or understandable ; getting 500k characters isn't. If I would like to see the entire dict/content of any object I can use vars() on it. it's designed for that purpose.

otherwise for your little point : you could add in the repr() the options passed to the read() call ; i.e :

repr(c.read(something, recursive=True)) => "... recursive=True .. " which would fix your point, isn't it ?

[gst]

also by the way, the convention is to use __class__.__name__ and not __class__ itself in

def repr(self):
    return "%s(whatever)" % self.__class__

eventually with the module name also included, or the qualname in python3+

But that's only for the purpose of being able to do things like eval(repr(obj)) == obj, which is mostly used only and eventually in tests (but personnaly that strikes me, I don't like and/or rely on that)