On Sat, Dec 8, 2012 at 5:46 PM, Shivani Poddar
<shivani.poddar92_at_gmail.com> wrote:
> 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
Thanks for your contribution.
Committed in r1418830.
I made some minor changes. Please consider these changes for future
contributions. One thing I find very helpful is to read through my
diffs while I write my commit message and look for typos, formatting
errors, unnecessary changes, etc...
- Removed some unnecessary whitespace changes in core.i
- Fixed the formatting in run_all.py to preserve spaces after commas
and wrap at 80 columns.
- Made several changes in checksum.py
* Cleaned up the imports. weakref and libsvn.core wasn't used. No
reason to import svn.core twice.
* Use the constant for the kind argument to svn_checksum_create
rather than a hardcoded value.
* Shortened up the variable name for the return from
svn_checksum_crate to make it easier to fit in 80 columns.
* Wrap to 80 columns.
* Fix indentation on comments and put a space after #
* Fix a typo of svn_checksum_to_cstring_display (avn instead of svn)
* Add license header to file.
- Adjusted the log to fit our log message standards:
http://subversion.apache.org/docs/community-guide/conventions.html#log-messages
* Space between * and filename.
* No space between filename and :
* Each file should have a separate * line.
* Mention function touched in run_all.py
* Don't mention generated files (checksum.pyc) that aren't committed.
* Multiple parties should be on separate lines for attribution lines.
* Reviewed by is redundant when I'm committing it (your inclusion
was fine since you didn't know I would be committing).
* Added Patch by line and note that I tweaked it slightly.
Suggestions for moving forward. The digest member of svn_checksum_t
is opaque to Python since unsigned char * isn't typemapped. You can
get at it with the display functions like your test does, but you
shouldn't have to.
I'd also recommend developing tests for all the checksum functions.
You'll find out what does and doesn't work along the way and then you
can start fixing them.
Received on 2012-12-09 09:24:09 CET