Re: Python bindings API confusion

On 2020/07/30 22:20, C. Michael Pilato wrote:
> The patch seems to work pretty well (for svn_repos_fs_commit_txn), but
> there are two issues I have with it.
>
> First, unless I'm mistaken, there's no way to get the conflict_p back as
> part of the 2-tuple return value -- if there's a conflict,
> svn.repos.fs_commit_txn() is going to raise an Exception. So we're
> basically returning a 2-tuple whose first value is always None. We might
> as well preserve the current behavior and return the newly committed
> revision as a singleton. (Which also means we don't break compatibility
> with prior versions.)

How do you think about this issue on svn.fs.commit_txn()? It is also
always (None, new_rev) if it doesn't raise Exception, since r845217,
Feb 2003. I also don't like this current behavor of svn.fs.commit_txn(),
however, I took priority of compatibility. (Though I think no one
ever used this Python wrapper function in practical program....)

Putting aside this compatibility issue, my preference is:

if error is not occured:
  if *conflict_p is NULL:
     return *new_rev as a singletone
  else:
     # a foolproof. we can detect an internal error by doing this.
     return tupple (*conflict_p, *new_rev)
else:
  # error is occred
  raise exception with values of *conflict_p and *new_rev in the exception

So I prefer to make this change and announce it. Note that we have
an option that we'll change behavor only in Python 3 with preserve
the behavor in Python 2.

> Also, I don't love that we've used "conflict_p" and "new_rev" as the
> variables names within the SubversionException. Anything we do here will
> be non-obvious and require documentation, so I'd rather give these things
> meaningful names ("conflict_path" and "new_revision"). But as a concept,
> I'd say this part works well.

Using argment names of C API have a merit that if we will implement such
special wrappers requires attirbutes of SubversionException, we just say
implement it, without attributes names. Although my patch uses
"conflict_p" and "new_rev" as a C string literal, it can be "$1_name"
and "$2_name" (in the case of svn.fs.commit_txn).

Of course, as there is no typemaps to be applied to multiple C APIs,
we should implement them one by one, so its trouble will be only a bit
that even we will name new attribute.
> Does this change anything about the svn_fs_commit_txn() interface?

Nothing in the current patch, except the exception also have those
attribute.

>
> -- Mike
>
> On Thu, Jul 30, 2020 at 5:00 AM Yasuhito FUTATSUKI <futatuki_at_yf.bsdclub.org>
> wrote:
>
>> On 2020/07/30 17:47, Yasuhito FUTATSUKI wrote:
>>> On 2020/07/29 23:43, C. Michael Pilato wrote:
>>>> Makes sense to me as a way to get access to all the things that can be
>>>> returned in an errorful situation.
>>>
>>> Okey, I implemented it. The attached patch add "conflict_p" and "new_rev"
>>> attributes to SubversionException instance on svn_fs.commit_txn and
>>> svn_repos.fs_commit_txn. Also, svn_repos.fs_commit_txn now returns
>>> tuple of (None, int), for a consistency.
>>>
>>> I've tested this method on other API with dummy attribute, but I didn't
>>> test on those svn_fs.commit_txn and svn_repos.fs_commit_txn.
>>>
>>> Could you please review and try it ?
>>>
>>> Thanks, --
>>> Yasuhito FUTATSUKI <futatuki_at_yf.bsclub.org>
>>
>> Ah, I sent the patch before including a commit message. Here it is.
>> [[[
>> swig-py: Allow SubversionException to add attributes
>>
>> [in subversion/bindings/swig/]
>>
>> * include/svn_types.swg
>> (svn_error_t_with_attrs): New C macro. An alias of svn_error_t to
>> distinct those C APIs which return value by using pointer args when
>> error is occured.
>> (typemap(out) svn_error_t_with_attrs *): New typemap.
>>
>> * python/libsvn_swig_py/swigutil_py.h, python/libsvn_swig_py/swigutil_py.c
>> (svn_swig_py_build_svn_exception): New function.
>> (svn_swig_py_svn_exception): Use svn_swig_py_build_svn_exception to
>> get the exception class and to make the instance of it from subversion
>> error.
>>
>> * svn_fs.i
>> (typemap(argout) (const char **conflict_p, svn_revnum_t *new_rev)):
>> - Add attribute "conflict_p" and "new_rev" to SubversionException
>> instance when the exception is occured.
>> - Fix the conversion format to build return value on Python 3
>> "z" conversion makes str when conflict_p is not NULL, although
>> it is always NULL in this context. So this is foolproof.
>> (svn_fs_commit_txn):
>> New fake function declaration for SWIG to make a wrapper. Ignore
>> the real one.
>>
>> * svn_repos.i
>> (typemap(argout) (const char **conflict_p, svn_repos_t *repos,
>> svn_revnum_t *new_rev)):
>> New typemap. Brought from typemap for svn_fs_commit_txn in svn_fs.i,
>> and modified argment number.
>> (svn_repos_fs_commit_txn):
>> New fake function declaration for SWIG to make a wrapper. Ignore
>> the real one.
>>
>> Found by: cmpilato
>> ]]]
>>
>>
>> Thanks,
>> --
>> Yasuhito FUTATSUKI <futatuki_at_yf.bsclub.org>
>>
>

Cheers,

-- 
Yasuhito FUTATSUKI <futatuki_at_yf.bsclub.org>