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. 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? > > 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. > > Great. :-) > > Cheers, > > Daniel > Troy
