On 12/17/18 2:08 AM, Troy Curtis Jr wrote:
On Sun, Dec 16, 2018 at 11:28 AM Daniel Shahaf <d...@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
