On Sat, Dec 8, 2012 at 1:51 AM, Ben Reser <ben_at_reser.org> wrote:
> On Thu, Dec 6, 2012 at 2:40 PM, Shivani Poddar
> <shivani.poddar92_at_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 <stsp_at_elego.de>
> > Ben Reser <breser_at_fornix.brain.org>
> > Daniel Shahaf <~danielsh_at_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
Received on 2012-12-07 21:43:57 CET