Skip to content

Conversation

@rhettinger
Copy link
Contributor

@rhettinger rhettinger commented Sep 16, 2017

@serhiy-storchaka serhiy-storchaka changed the title Issue 31482: Missing bytes support for random.seed() version 1 bpo-31482: Missing bytes support for random.seed() version 1 Sep 16, 2017
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a 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.

"""

if version == 1 and isinstance(a, (str, bytes)):
a = a.decode('latin-1') if isinstance(a, bytes) else a
Copy link
Member

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.

Copy link
Contributor Author

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.

# 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
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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?

@serhiy-storchaka serhiy-storchaka added the type-bug An unexpected behavior, bug, or error label Sep 16, 2017
@serhiy-storchaka
Copy link
Member

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.

@rhettinger rhettinger merged commit 132a7d7 into python:master Sep 17, 2017
@miss-islington
Copy link
Contributor

Thanks @rhettinger for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@bedevere-bot
Copy link

GH-3629 is a backport of this pull request to the 3.6 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 17, 2017
…ythonGH-3614)

bpo-31482: Missing bytes support for random.seed() version 1 pythonGH-3614
(cherry picked from commit 132a7d7)
@rhettinger rhettinger deleted the rand-seed-ver1-bytes branch September 17, 2017 16:04
@serhiy-storchaka
Copy link
Member

Commit message is duplicated.

@miss-islington
Copy link
Contributor

Thanks @rhettinger for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 19, 2017
…ythonGH-3614)

bpo-31482: Missing bytes support for random.seed() version 1 pythonGH-3614
(cherry picked from commit 132a7d7)
@bedevere-bot
Copy link

GH-3659 is a backport of this pull request to the 3.6 branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type-bug An unexpected behavior, bug, or error

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants