richardkiss / pycoin

Python-based Bitcoin and alt-coin utility library.
MIT License
1.4k stars 497 forks source link

removed the redundant __str__ method #407

Closed arnaudlegout closed 1 year ago

arnaudlegout commented 2 years ago

Just removing the redundant __str__ method as the __repr__ method is already implemented and contains the same code block.

richardkiss commented 2 years ago

I was away from my computer when you asked about this on Zulip. I notice that this PR causes the following:

>>> from pycoin.encoding.hexbytes import bytes_as_revhex
>>> t = bytes_as_revhex(b"foo")
>>> str(t)
"b'foo'"
>>> repr(t)
'6f6f66'

I'm pretty sure this is not what we want. Maybe it's the other way around -- you should remove the __repr__ implementation, not the __str__.

This is python 3.10.

richardkiss commented 2 years ago

Similar thing happens if we remove just __repr__.

>>> from pycoin.encoding.hexbytes import bytes_as_revhex
>>> t = bytes_as_revhex(b"foo")
>>> str(t)
'6f6f66'
>>> repr(t)
"b'foo'"

So I think both implementations are necessary. Let me know what you think @arnaudlegout

arnaudlegout commented 2 years ago

@richardkiss you are right. My bad for pushing a PR without appropriately testing it.

The bytes type implements both __str__ and __repr__, so we must overload them both.

>>> ('__repr__' in vars(bytes)) and ('__str__' in vars(bytes))
True

I made a new commit to fix the issue. I added a slight refactoring and comments. Feel free to drop it if you don't like it.

richardkiss commented 1 year ago

This was a nice idea, and I appreciate the thought.