-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
bpo-31482: Missing bytes support for random.seed() version 1 #3614
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
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.
Please add a NEWS.d entry. Sorry, I missed it.
@@ -110,6 +110,7 @@ def seed(self, a=None, version=2): | |||
""" | |||
|
|||
if version == 1 and isinstance(a, (str, bytes)): | |||
a = a.decode('latin-1') if isinstance(a, bytes) else a |
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.
Maybe instead convert a string to a list of integers and remove ord()
in the following code?
if isinstance(a, str):
a = list(map(ord, a))
This would make the following code clearer and maybe slightly faster.
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.
I prefer to leave the current code alone and just add a conversion for the bytes which is so uncommon that no one ever noticed the lack of bytes support.
Lib/test/test_random.py
Outdated
# Verify that version 1 seeds are unaffected by hash randomization | ||
# when the seeds are expressed as bytes rather than strings | ||
|
||
self.gen.seed(b'nofar', version=1) # hash('nofar') == 5990528763808513177 |
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.
hash('nofar') == 5990528763808513177
This is not true because of hash randomization.
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 comment referred to the Python2.7 hash() function (non-randomized). That is what is being emulated by version 1.
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.
Could you add a comment for this?
In general LGTM. But tests for string and bytes seeds test only ASCII strings. Would be nice to add tests for non-ASCII strings. For example if use encoding other than "latin-1" in your patch, this bug wouldn't be catched by existing tests. |
Thanks @rhettinger for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6. |
GH-3629 is a backport of this pull request to the 3.6 branch. |
…ythonGH-3614) bpo-31482: Missing bytes support for random.seed() version 1 pythonGH-3614 (cherry picked from commit 132a7d7)
Commit message is duplicated. |
Thanks @rhettinger for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6. |
…ythonGH-3614) bpo-31482: Missing bytes support for random.seed() version 1 pythonGH-3614 (cherry picked from commit 132a7d7)
GH-3659 is a backport of this pull request to the 3.6 branch. |
https://bugs.python.org/issue31482