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.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.
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).
Received on 2013-02-20 08:21:39 CET