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. 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. 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). 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). 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.
