Shivani Poddar wrote on Wed, Feb 20, 2013 at 04:00:34 +0530: > On Wed, Feb 20, 2013 at 2:41 AM, Daniel Shahaf <d...@daniel.shahaf.name>wrote: > > > Shivani Poddar wrote on Wed, Feb 20, 2013 at 00:28:41 +0530: > > > On Tue, Feb 19, 2013 at 11:55 PM, Ben Reser <b...@reser.org> wrote: > > > > > > > On Sat, Feb 16, 2013 at 4:26 AM, Shivani Poddar > > > > <shivani.podda...@gmail.com> wrote: > > > > > [ [ [ > > > > > > > > > > Improving support for svn_checksum.h in SWIG bindings. > > > > > > > > This isn't how I'd word this commit description. You haven't actually > > > > improved support at all with this patch, but rather you've added tests > > > > for a function that already works. That doesn't mean it's not a > > > > worthwhile contribution (it is), it just isn't what is described. > > > > > > > > > Index: subversion/bindings/swig/python/tests/checksum.py > > > > > =================================================================== > > > > > --- subversion/bindings/swig/python/tests/checksum.py (revision > > 1446877) > > > > > +++ subversion/bindings/swig/python/tests/checksum.py (working copy) > > > > > @@ -18,9 +18,10 @@ > > > > > # under the License. > > > > > # > > > > > # > > > > > +import sys > > > > > import unittest, setup_path > > > > > import svn.core > > > > > > > > Why import sys you're not using it anywhere? > > > > > > > > > > > Will check the way the above is worded in the patch and also, yes, I need > > > not import sys. > > > Maybe writing "adding tests for svn_checksum_dup() function in > > > svn_checksum.h would be the correct thing to do. > > > > > > > > > > > - > > > > > +LENGTH = > > > > > > svn.core.svn_checksum_size(svn.core.svn_checksum_create(svn.core.svn_checksum_md5)) > > > > > class ChecksumTestCases(unittest.TestCase): > > > > > def test_checksum(self): > > > > > # Checking primarily the return type for the > > svn_checksum_create > > > > > > > > I'd have probably put LENGTH at least inside the ChecksumTestCases > > > > class. There's no reason for it to be a global. But really I don't > > > > understand why it's here at all, see below for more on that. > > > > > > > > > > > Since in the earlier patches we had LENGTH as a global variable, I did > > not > > > feel the need to change it here. > > > > > > > > > > > > > > > > > @@ -28,7 +29,12 @@ class ChecksumTestCases(unittest.TestCase): > > > > > kind, expected_length = svn.core.svn_checksum_md5, 128/8 > > > > > val = svn.core.svn_checksum_create(kind) > > > > > check_val = svn.core.svn_checksum_to_cstring_display(val) > > > > > + is_duplicate = svn.core.svn_checksum_dup(val); > > > > > > > > is_duplicate implies to me that this is the result of a test to check > > > > if something is a duplicate, so on a glance I'd be expecting > > > > is_duplicate to be a boolean. But rather it's a svn_checksum_t. I'd > > > > have just named it duplicate. > > > > > > > > > > > Yes, didn't perceive that naming might indicate otherwise. > > > (Will rectify) > > > > > > > > > > > > > > > > + self.assertEqual(type(check_val),str,"Type of digest not > > > > string") > > > > > + self.assertEqual(len(check_val)%LENGTH,0,"Length of digest > > does > > > > not match kind") > > > > > + self.assertEqual(int(check_val),0,"Value of initialized > > digest > > > > is not 0") > > > > > + > > > > > self.assertTrue(isinstance(check_val, str), > > > > > "Type of digest not string") > > > > > self.assertEqual(len(check_val), 2*expected_length, > > > > > > > > Why is this here? Thought we were testing the duplicate, this code is > > > > testing the check_val and is essentially a duplicate of the the > > > > assertions it precedes. Modulus is not the right operation to test > > > > the length. In fact now that I think about this. This looks > > > > familiar... > > > > > > > > Yes these lines and the LENGTH appears to be from this patch: > > > > > > > > > > http://mail-archives.apache.org/mod_mbox/subversion-dev/201212.mbox/%3CCAFFzEcM2B_T55m2BQ95_2K_W=duxfkwdmdcjo8fctodyoku...@mail.gmail.com%3E > > > > > > > > Which was committed by danielsh with some modifications and then > > > > ultimately modified further based on some feedback on the list. > > > > > > > > > > > Yes, this is merely the same. I did not rewrite it this time around. I am > > > not sure why would these lines come with a (+) in the patch. > > > Using modulus here was to tackle the different types of > > svn_checksum_kind_t > > > we have. This was deliberated at earlier when danielsh reviewed that > > patch. > > > > As I recall, my point was that you can't hardcode 32 (or 128) for md5 > > and 40 (or 160) for sha1, and instead need to figure it out at runtime > > (for forward compat). But the checksum length will be 32*2, or 40*2, or > > $somefuturevalue*2 --- but it'll always be *2, since the number of > > hexdigits per byte doesn't depend on the kind. > > > > Correct, and so at that time , doing a modulus seemed to solve the purpose > of it.
The problem was solved by getting the expected length at runtime (because you can't hardcode either 128 or 160). Using that computed-at-runtime value as the right-hand side of the % operator (__mod__) is secondary (and, as I just finished saying, possibly unnecessary).
