On Wed, Feb 20, 2013 at 2:41 AM, Daniel Shahaf <d.s_at_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 <ben_at_reser.org> wrote:
> >
> > > On Sat, Feb 16, 2013 at 4:26 AM, Shivani Poddar
> > > <shivani.poddar92_at_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=DUXfKWdmdCjO8fctODyoku9tQ@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.
Received on 2013-02-19 23:31:07 CET