Based on the reviews for the earlier PATCH for the svn_checksum.h header's
python binding i have incorporated the required changes and finally have
shaped the earlier patch.
Please find the new patch file attached.
log message:
*subversion/bindings/swig/core.i: pulled in header svn_checksum.h
*subversion/bindings/swig/python/tests/:
checksum.py : New file
checksum.pyc: New file
run_all.py: Included a test_suite for checksum.py
Suggested by: breser, danielsh, stsp
Review by : breser
On Sat, Dec 8, 2012 at 2:13 AM, Shivani Poddar
<shivani.podda...@gmail.com>wrote:
>
>
> On Sat, Dec 8, 2012 at 1:51 AM, Ben Reser <b...@reser.org> wrote:
>
>> On Thu, Dec 6, 2012 at 2:40 PM, Shivani Poddar
>> <shivani.podda...@gmail.com> wrote:
>> > Log Message:
>> > subversion/bindings/swig/include
>> > (svn_checksum_t.swg) : Generated a typemap.
>> > (svn_checksum_kind_t,svn_checksum_create,svn_checksum_clear): typemapped
>> > these functions
>> >
>> > Suggested by: Stefan Sperling <s...@elego.de>
>> > Ben Reser <bre...@fornix.brain.org>
>> > Daniel Shahaf <~danielsh@apache>
>>
>> In this case I'd say that the three people you're providing
>> attribution to here are all committers. Their attribution can be
>> shortened to just their usernames. Which are: stsp, breser, danielsh.
>>
>> > I hope this PATCH is incorporated and i am able to contribute
>> substantially
>> > to the subversion community.
>>
>> In addition to the things Daniel already brought up, some comments on
>> your patch.
>>
>> 1) The most critical piece of feedback I can give you here is that you
>> need to ensure that your patch is functional and achieves the stated
>> end. Which means at a minimum you need to write some Python code that
>> uses the interfaces you have exposed with the bindings. The Python
>> bindings have a test suite, I'd advise that you add code that
>> exercises the interface. This means that not only have you
>> demonstrated that it works to yourself but we are able to ensure that
>> it stays working.
>>
>> Yes i had a mental note to check this after Daniel mailed in. I will make
> sure i work on that.
>
>
>> 2) You've created a new checksum module. Why? As I tried to explain
>> on IRC we break the modules up by the Subversion libraries. There is
>> a somewhat out of date NOTES document in the subversion/bindings/swig
>> directory that explains this. You can find it online here:
>>
>> http://svn.apache.org/repos/asf/subversion/trunk/subversion/bindings/swig/NOTES
>>
>> In my opinion svn_checksum.h should be added to _core which is used
>> for things in libsvn_subr and that don't fit anywhere else.
>>
>> I tried running it by injecting my typemaps into a .i file which required
> the insertion of a module, i wasnt sure if this were the right way to do
> it, i could not get in any documentations citing the same directly, maybe
> there is a need for me to dig in more regarding this issue.
>
>
>> 3) You have two "%typemap(in) svn_checksum_t *" definitions. I don't
>> understand what you need this for. You should only need one typemap
>> for a given type and SWIG method ('in' is the SWIG method).
>>
>
>
> I should probably have written the later one only.I wanted to turn in
> both the typesmaps i have generated , the svn_checksum_clear being the
> first went in as it is while the svn_checksum_create came in later, i did
> not however anticipate that this would pose any problems since if the
> typemap did not find its required function in the first instance it would
> still keep looking and encounter the later case,This i think now was a
> faulty assumption to make.
>
>
>>
>> 4) The typemaps shouldn't need to call svn_checksum_create() or
>> svn_checksum_clear(). Typemaps just convert types between C and the
>> Binding Language. It's best to think of the typemap as code that is
>> injected into the wrapper that SWIG is building, where in the wrapper
>> it is injected depends on the method of the typemap. Refer to the
>> SWIG documentation I pointed you at on IRC.
>>
>> In this case maintaining the struct and providing accessor functions
>> to it would be the right way to go about this. Simply converting to a
>> string won't be very helpful since you'll be throwing away the kind of
>> checksum (something Daniel has already mentioned).
>>
>
> If we do not take in consideration the svn_checksum_clear i believe i have
> tried prserving the struct, since in create i am returning the integer
> which convers the kind, i havent passed it as $input later since it remains
> fixed throughout the function. Please point if my method of doing this in
> svn_checksum_create is faulty.
>
>>
>> 5) As I'd mentioned on IRC the major things you have to write a
>> typemap for is argouts and callbacks (there are some other minor
>> cases). There are svn_checksum_t's that are used as output arguments
>> so you do need to implement the argout. However, the struct and the
>> enum should just be handled automatically for you by SWIG in the other
>> cases. You may have to do some work to enable accessor functions for
>> the struct members, I'm not sure what's needed on the Python side for
>> that. Which leads me to ...
>>
>> 6) Taking a look at the checksum related functions and types I see
>> that the symbols aren't showing up to SWIG. So someone needs to pull
>> in those symbols into a SWIG module. That should be a simple matter
>> of adding the auto-generated .swg file for the header file to the
>> module.
>>
>> The following comments are nitpicky comments about your patch. Given
>> the above you need to majorly rework the patch in order for it to be
>> functional. However, I still mention these things since we do try to
>> maintain code quality and a functional patch with these sorts of
>> issues will likely receive these sorts of comments.
>>
>> 1) You start a comment on the same line at the end of the checksum
>> module definition. I would put this on a separate line.
>> 2) svn_checksum_clear is mistyped as avn_checksum_clear.
>> 3) Your patch file has just the filename, instead of the full path.
>> We typically run svn diff from the top level of the wc so as to make
>> your patch easier to apply.
>> 4) The log message is not in the proper format per the community
>> guide:
>> http://subversion.apache.org/docs/community-guide/conventions.html#log-messages
>>
>> I'll be around this weekend so if you have any questions, please feel
>> free to ask. I'll do my best to help you get your patch in shape so
>> that it can be committed.
>>
>
>
> Thanks a lot for you feedback.
> Yes i would want to work on it and get it in shape this weekend , that
> would be great :)
>
> Regards,
> Shivani Poddar
>
--
Shivani Poddar,
Bachelors in Computer Sciences and MS in Exact Humanities, Sophomore
International Institute of Information Technology, Hyderabad
Index: subversion/bindings/swig/core.i
===================================================================
--- subversion/bindings/swig/core.i (revision 1416225)
+++ subversion/bindings/swig/core.i (working copy)
@@ -786,9 +786,8 @@
%include svn_dirent_uri_h.swg
%include svn_mergeinfo_h.swg
%include svn_io_h.swg
+%include svn_checksum_h.swg
-
-
#ifdef SVN_AUTH_PARAM_GNOME_KEYRING_UNLOCK_PROMPT_FUNC
%inline %{
/* Helper function to set the gnome-keyring unlock prompt function. This
Index: subversion/bindings/swig/python/tests/run_all.py
===================================================================
--- subversion/bindings/swig/python/tests/run_all.py (revision 1416225)
+++ subversion/bindings/swig/python/tests/run_all.py (working copy)
@@ -19,7 +19,7 @@
#
#
import unittest, setup_path
-import mergeinfo, core, client, delta, pool, ra, wc, repository, auth, \
+import mergeinfo, core, client, delta,checksum, pool, ra, wc, repository,
auth, \
trac.versioncontrol.tests
# Run all tests
@@ -28,6 +28,7 @@
"""Run all tests"""
s = unittest.TestSuite()
s.addTest(core.suite())
+ s.addTest(checksum.suite())
s.addTest(mergeinfo.suite())
s.addTest(client.suite())
s.addTest(delta.suite())
Index: subversion/bindings/swig/python/tests/checksum.py
===================================================================
--- subversion/bindings/swig/python/tests/checksum.py (revision 0)
+++ subversion/bindings/swig/python/tests/checksum.py (revision 0)
@@ -0,0 +1,27 @@
+import unittest,weakref, setup_path
+import svn.core,libsvn.core
+from svn.core import *
+value=1
+class ChecksumTestCases(unittest.TestCase):
+ def test_checksum(self):
+#Checking primarily the return type for the svn_checksum_create function
+ svn_checksum_t_val = svn.core.svn_checksum_create(value)
+ check_val =
svn.core.svn_checksum_to_cstring_display(svn_checksum_t_val)
+#The avn_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)
+ else:
+ self.assertRaises(TypeError, test_checksum)
+
+def suite():
+ return unittest.defaultTestLoader.loadTestsFromTestCase(ChecksumTestCases)
+if __name__ == '__main__':
+ runner = unittest.TextTestRunner()
+ runner.run(suite())
+
+
+
+
