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

Add random.sampleBuffer #16957

Closed
wants to merge 41 commits into from
Closed

Add random.sampleBuffer #16957

wants to merge 41 commits into from

Conversation

juancarlospaco
Copy link
Collaborator

@juancarlospaco juancarlospaco commented Feb 6, 2021

  • Add random.randToken.
  • Inspired but not copied from Python 3.9+ secrets.token_urlsafe()
  • ASCII by default.
  • URL-Safe by default.
  • Customizable length and set[char].
  • Documentation with runnableExamples, since, changelog, tiny diff.
  • Python generates a random string then calls base64 encode to make it URL safe, my implementation do not use base64.

Notes

@juancarlospaco juancarlospaco changed the title WIP Add random.randToken Feb 6, 2021
@juancarlospaco juancarlospaco marked this pull request as ready for review February 6, 2021 23:12
@juancarlospaco
Copy link
Collaborator Author

@timotheecour Can you review; thanks.

@juancarlospaco
Copy link
Collaborator Author

I agree, if you are a Nim core dev is very easy to implement, optimize and test, one more reason why it should be implemented,
new users often struggle with something is stdlib builtin and easy one-liner on Python:

:)

@timotheecour
Copy link
Member

timotheecour commented Feb 7, 2021

how about the following instead, which is more generic but not harder to use (compared a version of your PR that would add r: var Rand), and maintains separation of concern (std/random shouldn't deal with base64 encoding issues)

proc sampleBuffer*[T](r: var Rand, buffer: var openArray[T]; alphabet: set[T]) {.inline, since: (1, 5).} =
  runnableExamples:
    import sugar
    doAssert newString(8).dup(sampleBuffer(randState, _, {'0'..'9'})).len == 8
    import base64
    doAssert newString(8).dup(sampleBuffer(randState, _, cb64safe)).len == 8
  for c in buffer.mitems: c = sample(r, alphabet)

pros/cons:

  • yes, it's easy to implement in one's code (but not as 1 liner)
  • common enough, and this is something frequently asked in other languages, there's a library way or not
  • doesn't add any meaningful complexity to std/random

@juancarlospaco juancarlospaco changed the title Add random.randToken Add random.sampleBuffer Feb 7, 2021
changelog.md Outdated Show resolved Hide resolved
Co-authored-by: flywind <43030857+xflywind@users.noreply.github.com>
@Clyybber
Copy link
Contributor

Clyybber commented Feb 8, 2021

how about the following instead, which is more generic but not harder to use (compared a version of your PR that would add r: var Rand), and maintains separation of concern (std/random shouldn't deal with base64 encoding issues)

proc sampleBuffer*[T](r: var Rand, buffer: var openArray[T]; alphabet: set[T]) {.inline, since: (1, 5).} =
  runnableExamples:
    import sugar
    doAssert newString(8).dup(sampleBuffer(randState, _, {'0'..'9'})).len == 8
    import base64
    doAssert newString(8).dup(sampleBuffer(randState, _, cb64safe)).len == 8
  for c in buffer.mitems: c = sample(r, alphabet)

pros/cons:

* yes, it's easy to implement in one's code (but not as 1 liner)

* common enough, and this is something frequently asked in other languages, there's a library way or not

* doesn't add any meaningful complexity to std/random

AFAICT your procs implementation is a one liner. The most complex part of this PR is the alphabet, so adding this makes even less sense IMO

@Araq
Copy link
Member

Araq commented Feb 8, 2021

I don't mind a Python inspired "secrets" module but random.nim isn't it:

Do not use this module for cryptographic purposes!

@Araq Araq closed this Feb 8, 2021
@juancarlospaco
Copy link
Collaborator Author

@Araq You dont like the proc ?, or you want it to be on a new module ?.

@Araq
Copy link
Member

Araq commented Feb 8, 2021

New module but ensure it's good enough for crypto.

@ringabout
Copy link
Member

Just like the secrets module of Python, it should be based on system random
#16459

@timotheecour
Copy link
Member

timotheecour commented Feb 12, 2021

@xflywind how about instead keeping it in std/random, but changing randomize() to use the fresh new std/sysrandom, then users can choose depending on how they call:

# CSPRNG
var r = initRand(opt = useSysrand)
sampleBuffer(r, buf, alphabet)

# PRNG
var r = initRand(opt = useTime)
sampleBuffer(r, buf, alphabet)

# CSPRNG
randomize() # could be changed to: randState = initRand(opt = useSysrand)
sampleBuffer(randState, buf, alphabet) # uses default RNG

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.

5 participants