[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] Improve svn_checksum_t bindings in SWIG

From: Daniel Shahaf <danielsh_at_elego.de>
Date: Tue, 11 Dec 2012 00:38:20 +0200

Shivani Poddar wrote on Tue, Dec 11, 2012 at 02:22:28 +0530:
> Log Message:
>
> Improve support for svn_checksum.h in SWIG bindings
> * subversion/bindings/swig/python/tests/checksum.py: Improved test_checksum
>

Need a blank line before the * line, and to use the "* file\n (symbol)"
syntax --- 'test_checksum' is a symbol.

> Review by: danielsh
>

That's inappropriate; I haven't reviewed the patch yet. You might add
this field after I review it, not before.

> Index: subversion/bindings/swig/python/tests/checksum.py
> ===================================================================
> --- subversion/bindings/swig/python/tests/checksum.py (revision 1419694)
> +++ subversion/bindings/swig/python/tests/checksum.py (working copy)
> @@ -20,22 +20,25 @@
> #
> import unittest, setup_path
> import svn.core
> -
> +LENGTH = 32 or 40;

This is wrong in two different ways:

- "32 or 40" is a constant expression that evaluates to 32.

- You hardcode two values, when you should be hardcoding neither of
  them. (The bindings shouldn't need to change if the C library grows
  a third svn_checksum_kind_t.)

> class ChecksumTestCases(unittest.TestCase):
> def test_checksum(self):
> # Checking primarily the return type for the svn_checksum_create
> # function
> val = svn.core.svn_checksum_create(svn.core.svn_checksum_md5)
> check_val = svn.core.svn_checksum_to_cstring_display(val)
> -
> # The svn_checksum_to_cstring_display should return a str type object
> # from the check_val object passed to it
> if(type(check_val) == str):
> - # The intialized value created from a checksum should be 0
> - if(int(check_val) != 0):
> - self.assertRaises(AssertionError)
> + #Check the length of the string check_val
> + if(len(check_val) == LENGTH):
> + # The intialized value created from a checksum should be 0
> + if(int(check_val) != 0):
> + raise

This bare "raise" statement without arguments is itself an error.

See for yourself:

    % python -c 'raise'
    TypeError: exceptions must be old-style classes or derived from BaseException, not NoneType

This exception signifies a bug in your program. It has become
a RuntimeError in recent Pythons (and, frankly, could become
a compile-time error as well --- the compiler knows there's no except:
block surrounding this statement). It might work, but not because it's
correct.

Daniel

> + else:
> + raise
> else:
> - self.assertRaises(TypeError, test_checksum)
> + raise
>
> def suite():
> return unittest.defaultTestLoader.loadTestsFromTestCase(ChecksumTestCases)
Received on 2012-12-10 23:39:17 CET

This is an archived mail posted to the Subversion Dev mailing list.