Re: [swig-py3][patch] interfacing bytes object instead of str

On 12/17/18 2:08 AM, Troy Curtis Jr wrote:
> On Sun, Dec 16, 2018 at 11:28 AM Daniel Shahaf <d.s_at_daniel.shahaf.name>
> wrote:
>
>> Troy Curtis Jr wrote on Sun, 16 Dec 2018 09:59 -0500:
>>> 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,
>>
>> Are you sure about this? Property values can be binary data (so not
>> representable as 'str') but are usually represented by «svn_string_t *»;
>> for example, svn_repos_parse_fns3_t::set_node_property().
>>
>
> Ah yes, I suppose this also has bearing on the discussion in the other
> thread relating
> to returning property values. With that in mind, perhaps your suggestion
> of effectively
> defaulting to always being Bytes for char * , svn_string_t, and
> svn_stringbuf_t unless there
> is a specific circumstance not to makes the most sense as the general
> principle.

To treat all those values as bytes in wrapper makes the wrapper is lower
level, keeping raw access like device drivers. On the other hand,
to make contexts, to make semantic differences in wrapper makes it more
higher level. So I think those are not the general principle but just a
policy of the wrapper design. I, as a consumer of the Python bindings,
prefer the latter if it is clear that what are bytes and what are str,
however I also agree with former if the distinction is too complex for
both of consumers and developers.

> Yasuhito, I suppose that means we should probably tweak the typemaps to
> follow this
> principle. Have you already started down that path based on the properties
> discussion?

Yes, but there is almost nothing to output, so you don't need to care :)
I've also read swig document again, especially about default typemaps,
and also there is no result :)

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

I think you are right. And svn_stringbuf_t * type don't have proxy class,
it is no mean to pass string prepend to the result. The consumer of those
APIs can write a code like (using application_pool):

    modules += svn.fs.svn_fs_print_modules()

However, this seems to be not only the issue swig-py3 but also trunk.
Actually both of svn.fs.svn_fs_print_module('') and
svn.ra.svn_ra_print_module('') return None type, on FreeBSD package for
Subversion 1.8.8 and 1.11.0, without patch to tweak swig codes, so
it seems those API wrappers are broken.

>>> 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 found exception error messages isn't unified for
('a bytes', 'a bytes object', 'a Bytes object'),
and nothing but these.

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

Oh, thank you!

Cheers,

-- 
Yasuhito FUTATSUKI