Re: Python bindings API confusion

On 2020/12/31 8:59, Yasuhito FUTATSUKI wrote:
> On 2020/12/28 19:10, Daniel Shahaf wrote:
>> Yasuhito FUTATSUKI wrote on Sat, 26 Dec 2020 11:02 +00:00:
>>> On 2020/12/26 3:38, Nathan Hartman wrote:
>>>
>>>> But the solution you suggest above, adding a new_rev attribute to the
>>>> exception and leaving the return value as it was, sounds like a good way to
>>>> avoid the incompatibility and also avoid the API bump.
>>>
>>> I had written such a patch on typemaps at first because it was straight
>>> forward. Although I removed it after r1880967 is commited, I can write
>>> such a code again and I'll do if it is needed.
>>>
>>> Alternately, here is an easier way to do it. The patch below is a code
>>> that replace wrapper function entity on top of the svn.fs module
>>> (not tested, and more appropriate comment is needed in the code).
>
> <snip>
>
>>> Of course, we can use svn_fs_commit_txn2 instead of _svn_fs_commit_txn
>>> in above patch, then it will satisfy the Daniel's proposal.
>>
>> I agree with Nathan that it'd be confusing for a Python
>> svn_fs_commit_txn2() to exist when a C function of the same name does
>> not, even if the C API skips a revv number in consideration of swig-py.
>> So, if we'd like to introduce a simplified API, I'd vote to name it
>> something else.
>
> Then I'll commit patch amended only for restore Python 2 support,
> without renaming to svn_fs_commit_txn2, if Mike agrees because he
> already wrote some code using svn.fs.commit_txn().
> (attached swig-py-fs_commit_txn-revert-return-value-type-patch.txt)

I got a reply from Mike off the list with non negative answer. So I think
there is no objection to restore the behavior on return value type of
svn.fs.commit_txn(), yet, at least.

However as a result of reconsideration of how it should be done, now
I don't think this patch wasn't sufficieant. If C API svn_fs_commit_txn()
returns SVN_NO_ERROR but conflict_p is not NULL by accident,
svn.fs.commit_txn() with this patch returns (None, (conflict, new_revision))
and this is obiously incorrect. So I added the check for it.
Also, I rerote the comment, then commit it in 1885007.

> I also wrote tests for svn.fs.commit_txn, both for return value and
> attributes in SubversionException, by porting tests for C API into Python.
> (attached patch swig-py-fs_commit_txn-test-patch.txt; tested on Python
> 2.7.18 and 3.9.0 on FreeBSD 12)

I added to more assertion to check if attributes exist in exception,
and commit it in 1885008.

Cheers,

-- 
Yasuhito FUTATSUKI <futatuki_at_yf.bsclub.org>