Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

removed the redundant __str__ method #407

Closed
wants to merge 2 commits into from

Conversation

arnaudlegout
Copy link
Contributor

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

@richardkiss
Copy link
Owner

richardkiss commented Oct 29, 2022

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
Copy link
Owner

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

…t be implemented. Slightly refactored and commented.
@arnaudlegout
Copy link
Contributor Author

@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
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants