[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: Python bindings API confusion

From: C. Michael Pilato <cmpilato_at_red-bean.com>
Date: Thu, 30 Jul 2020 09:20:49 -0400

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

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.

Does this change anything about the svn_fs_commit_txn() interface?

-- Mike

On Thu, Jul 30, 2020 at 5:00 AM Yasuhito FUTATSUKI <futatuki_at_yf.bsdclub.org>

> 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>
Received on 2020-07-30 15:21:35 CEST

This is an archived mail posted to the Subversion Dev mailing list.