On 2020/07/31 10:19, Yasuhito FUTATSUKI wrote:
> 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 2-tuple (*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.
I've revised the patch, with this applying to both of Python 2 and Python 3.
>> 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.
In this patch, I used "$1_name" yet.
Cheers,
--
Yasuhito FUTATSUKI <futatuki_at_yf.bsclub.org>
Received on 2020-08-02 15:37:01 CEST