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

can u pullz? #2

Closed
wants to merge 3 commits into from
Closed

can u pullz? #2

wants to merge 3 commits into from

Conversation

btoconnor
Copy link

I added a doctest that was missing, converted a few text examples to doctests.

additionally, i also changed your random_string function to utilize os.urandom, which is supposed to be better.

From the python docs (as we discussed some time ago):

Almost all module functions depend on the basic function random(), which generates a random float uniformly in the semi-open range [0.0, 1.0). Python uses the Mersenne Twister as the core generator. It produces 53-bit precision floats and has a period of 2**19937-1. The underlying implementation in C is both fast and threadsafe. The Mersenne Twister is one of the most extensively tested random number generators in existence. However, being completely deterministic, it is not suitable for all purposes, and is completely unsuitable for cryptographic purposes.

Anyway, pull if you wish.

@shazow
Copy link
Owner

shazow commented Jul 30, 2011

Please split the pull request into a separate one for each topic, so I can accept them individual or ask for changes when necessary.

Some feedback:

  1. I don't want to accept your changes to random_string for two reasons: It breaks for alphabets bigger than 255 and it's more complicated and not as clear as the original version which goes against the philosophy of the library (namely "We value simplicity and elegance over robustness and optimization.").
  2. I appreciate the doctests fixes, please include them in a separate pull request. Any particular reason for changing "Example" to "For example"? Is there some sort of standard for this? If not, I'd prefer to leave it as-is to avoid bouncing back and forth.
  3. The pop_many docstring is good, thank you. Please include it as a separate pull request. :)

@shazow shazow closed this Jul 30, 2011
@btoconnor
Copy link
Author

I have no idea how to split up commits for separate pull changes, but I guess I'll have to figure it out. This is my first adventure on github.

  1. That's fine. I'm going to keep it in mine - that's what I use in my lib.
  2. Actually, I took the 'For Example' bit from one of the existing doctests, and kept it for consistency. If you want consistency - which would you like ? 'Example', or 'For Example'?
  3. Will do, as soon as I figure out how to.

@shazow
Copy link
Owner

shazow commented Jul 30, 2011

Let's do Example, less writing. :P

I think to split them up, you need to make a separate upstream branch (based
on a revision that exists in the origin repo) for every changeset and
request merges with said branches.

Let me know if you figure it out.

Also re 1. You're welcome to add a note in the docstring in the end that the
randomness could be improved by using os.urandom.

On Fri, Jul 29, 2011 at 8:57 PM, btoconnor <
reply@reply.github.com>wrote:

I have no idea how to split up commits for separate pull changes, but I
guess I'll have to figure it out. This is my first adventure on github.

  1. That's fine. I'm going to keep it in mine - that's what I use in my
    lib.
  2. Actually, I took the 'For Example' bit from one of the existing
    doctests, and kept it for consistency. If you want consistency - which
    would you like ? 'Example', or 'For Example'?
  3. Will do, as soon as I figure out how to.

Reply to this email directly or view it on GitHub:
#2 (comment)

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.

None yet

2 participants