On Wed, Nov 28, 2018 at 10:40 AM Daniel Shahaf <d.s_at_daniel.shahaf.name>
wrote:
> Yasuhito FUTATSUKI wrote on Tue, Nov 27, 2018 at 06:50:46 +0900:
> > I've revised typemaps and APIs using svn_stringbuf_t *, then I found
> > almost all those APIs include svn_stream_readline() use svn_stringbuf_t
> > for file contents. So I've modified typemaps again so that those APIs
> > use bytes for svn_stringbuf_t interface.
> >
> > The patch below destined for branches/swig-py affects those API wrappers.
>
> Thanks for the patch, Yasuhito. It looks good on a quick skim. Troy,
> you've
> worked on this branch before; would you perchance be able to review this?
> (If you can, great; but no worries if you can't)
>
Yes the patch is looking good, especially having the svn_stream Python APIs
actually tested! I updated the patch so that it works with the latest
swig-py3 (@r1849027) and attached it.
But there was one item I wanted to talk about related to the patch. I
agree that "svn_stringbuf_t" => "bytes" and "svn_string_t" => "str" is the
right general path, but after the patch there is now a "svn_stringbuf_t
*output" typemap that is still using "PyStr" instead of "PyBytes".
Further, the only usage of "svn_stringbuf_t *output" is in
"svn_fs_print_modules" and "svn_ra_print_modules", where the function
parameter is actually an output parameter, so the appropriate typemap is
probably "argout" instead of "in".
Otherwise, the patch LGTM. Yasuhito, if you want to review my changes to
your patch and perhaps address the issue in the last paragraph then I can
commit your patch to swig-py3. I'll also start working more on the swig-py3
branch to get it to the point that we can actually get this branch merged
into trunk finally.
Troy
Received on 2018-12-16 16:00:06 CET