-
Notifications
You must be signed in to change notification settings - Fork 23
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
use tobytes instead of tostring #550
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Failing test is because this PR comes from a fork rather than a branch and can be ignored since lib does not contain pipeline code.
@@ -808,7 +808,7 @@ def read_str(self, l, enc="ascii"): | |||
""" | |||
a = array.array("b") | |||
a.fromfile(self.f, l) | |||
return a.tostring().decode(enc) | |||
return a.tobytes().decode(enc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
based on the fact that here is a .decode(enc)
following the tostring()
call, I assume the result already was a byte-string and we do not need to change anything else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is tostring is removed for Python >=3.9, which means calling it directly results in an error. I assume since we are gradually shifting to higher Python versions and Python <3.2 is hardly used, it might make sense to change? But we can of course also just require people using lower Python version when calling the rasr_cache, not a problem for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for clarifying: tostring and tobytes are absolutely the same thing with just different names, see here I don't know why they did it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and we do not need to change anything else?
I meant "change additional code because instead of a str we now get a bytes object" (which is not the case). I am totally in favor of the change.
tostring in array.array has been replaced by tobytes and deprecated since Python 3.2 and is already removed in Python 3.9. I therefore propose to change the code to avoid this problem.