[pycrypto] Initial review of Thorsten's Py3k changes
sbehrens at gmx.li
Mon Jan 10 09:18:54 CST 2011
Thanks for looking over my commits.
I have been trying to rebase and am getting nowhere fast. There are a
lot of merge conflicts, and I end up with something that may or may not
actually be the current state of the repository.
> Therefore, I have a few recommendations:
> - Don't change the test vector data if you don't need to. Changing
> hundreds of strings like 'd6a141a7ec3c38dfbd61' to strings like
> b('d6a141a7ec3c36dfbd61') is unnecessary (since they need to be
> hex-decoded somewhere anyway), and it makes it difficult to tell by
> inspection that the test vectors weren't modified. (How many of you
> noticed that I changed the 8 to a 6 in that example?)
This was done for a reason. The tests were failing because b'something'
does not equal 'something', and
because several functions expected bytes, but got str.
As far as I could make out, this was the best way forward. The
alternative may have been to write things like
assertEqual(b(x),y) and adding b() to various function calls throughout
the test suite.
instead of leaving the asserts and function calls (largely) the way they
Adding b() to asserts and function calls struck me as rather opaque. I
thought it more consistent to change the way the test vectors are
I get that this makes review hard. I didn't think of that aspect at the
I'm not sure what the best way forward is for this part of the changes.
I still think it's cleaner to change the literals than the way asserts
and functions are called. b(s) in Py3k returns s.encode("latin-1").
Compare and contrast these two:
input = b'abcdef00'
expected = b'abcdef00'
x = somefunction(input)
input = 'abcdef00'
expected = 'cdefab00'
x = somefunction(input.encode("latin-1"))
If you were to write native Py3k code, you'd choose the former over the
latter. I tried to get as close to that as I could. I don't quite have
b'something', since I can't do that and remain compatible with Python
2.x. But the spirit of it is intact: I am changing the way the literal
is presented, instead of working with a string literal and changing it
to bytes whenever I use it.
> - Run your tests with python -tt to ensure consistent use of whitespace. I
> haven't been doing this, so the current master doesn't work, but that was
> a mistake and I will be doing it from now on.
> - The commands "git rebase -i", "git cherry-pick -n", "git reset", and
> "git add -p" are your friends.
I think I need some git schooling. I am not getting anywhere fast :/.
> - References to things like RC5 or IDEA, which have been removed from
> PyCrypto, can be removed.
I didn't touch those since they may have been there for a reason.
> - If adding additional/alternative dependencies like MPIR, include *why*
> that's being done in the commit message and/or in the documentation.
Hmm, I thought I did. This was done so _fastmath.c would work on
Windows. GMP is actively hostile to compilation on Windows. MPIR is a
GMP fork that is friendly to compilation on Windows.
More information about the pycrypto