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.
Received on 2013-02-19 22:18:50 CET