subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Troy Curtis Jr <troycurti...@gmail.com>
Subject Re: svn commit: r1818995 - in /subversion/branches/swig-py3: build/ac-macros/swig.m4 subversion/bindings/swig/include/proxy.py subversion/bindings/swig/include/proxy.swg
Date Sun, 31 Dec 2017 02:05:23 GMT
On Sat, Dec 30, 2017 at 12:49 PM Branko Čibej <brane@apache.org> wrote:

> On 30.12.2017 16:11, Troy Curtis Jr wrote:
> >
> >
> > On Fri, Dec 29, 2017 at 11:46 PM Daniel Shahaf <d.s@daniel.shahaf.name
> > <mailto:d.s@daniel.shahaf.name>> wrote:
> >
> >     Good morning Troy,
> >
> >     troycurtisjr@apache.org <mailto:troycurtisjr@apache.org> wrote on
> >     Fri, Dec 22, 2017 at 03:52:59 -0000:
> >     > On branch swig-py3: Correctly manage swig objects with Python
> >     new style classes.
> >     >
> >     > * build/ac-macros/swig.m4
> >     >   (SVN_FIND_SWIG):
> >     >   Request that swig generate new style classes, even for Python
> >     2, to reduce
> >     >   differences with Python 3.
> >     >
> >     > Modified: subversion/branches/swig-py3/build/ac-macros/swig.m4
> >     > URL:
> >
> http://svn.apache.org/viewvc/subversion/branches/swig-py3/build/ac-macros/swig.m4?rev=1818995&r1=1818994&r2=1818995&view=diff
> >     >
> >
>  ==============================================================================
> >     > --- subversion/branches/swig-py3/build/ac-macros/swig.m4 (original)
> >     > +++ subversion/branches/swig-py3/build/ac-macros/swig.m4 Fri Dec
> >     22 03:52:59 2017
> >     > @@ -143,7 +143,7 @@ AC_DEFUN(SVN_FIND_SWIG,
> >     >          if test "$ac_cv_python_is_py3" = "yes"; then
> >     >             SWIG_PY_OPTS="-python -py3"
> >     >          else
> >     > -           SWIG_PY_OPTS="-python -classic"
> >     > +           SWIG_PY_OPTS="-python"
> >     >          fi
> >
> >     What are the compatibility implications of this change?  I
> >     couldn't find any
> >     documentation for the "-classic" option.
> >
> >     > * subversion/bindings/swig/include/proxy.py
> >     >   (__getattr__): Replace __getattr__ with __getattribute__ to
> >     correctly
> >     >   intercept attribute access, even when swig uses descriptors
> >     for new style
> >     >   classes (which are the only type available in Python 3).
> >
> >     Could you help me understand what's going on here and why it's
> >     correct?  I've
> >     read the documentation of object.__getattr__ and
> >     object.__getattribute__¹ but I
> >     don't follow how the fact that the class into which this code is
> >     included
> >     became a new-style class (rather than a classic class) brought on
> >     the need to
> >     migrate from __getattr__ to __getattribute__, nor why the order of
> >     precedence
> >     of lookup (first __dict__, then object.__getattribute__, then
> >     _swig_getattr) is
> >     correct.
> >
> >     I'm asking because I'm trying to review the branch (to +1 its
> >     merge) and this
> >     is one of the open questions I have.
> >
> >     ¹
> >
> https://docs.python.org/3/reference/datamodel.html#object.__getattr__
> >     et seq.
> >
> >
> > Sure thing.  This is the change that took me so long to find, and then
> > fix the right way, so I certainly understand the non-obviousness of
> > it.  Perhaps some portion of this explanation could go in an
> > appropriate place in the code for future reference?  Though much of it
> > only applies to the motivation for the change and probably wouldn't be
> > beneficial just given the final implementation.
> >
> > Giving swig the '-classic' option forces it to generate only classic
> > classes.  Without that flag it will produce code that works as either
> > an old style or new style class, dependent on a try/except block
> > around 'import object'.  In practice, this means that for any Python
> > past 2.2 (when new-style classes were introduced), the classes are in
> > the new style and inherit from 'object'.
>
> This all makes sense and seems nice on the surface, but I'm not sure we
> can just change the behaviour of the bindings from old-style to
> new-style classes in a Python 2.x build. There are enough subtle
> differences in behaviour between the two that existing scripts could
> break after an upgrade of the bindings.
>
> Python 3.x has only new-style (or rather, even-newer-style) classes and
> there's no backward-compatibility consideration, since our bindings
> currently don't work with Python3.
>
>
That is a reasonable concern.  I definitely preferred the cleaner single
implementation, but honestly the code necessary to continue to use classic
classes in python 2 is not large.  I've attached a working patch for
reference/discussion.  It is a bit more code and some conditional
definitions, but perhaps it is the more preferred course to take?

[[[
On branch swig-py3: Go back to using classic classes for Python 2 swig
bindings.

Add some additional clarifying comments for the reasons behind overriding
__getattr__ and __getattribute__.

* build/ac-macros/swig.m4
  (SVN_FIND_SWIG): Add the '-classic' flag to swig when python 2 is
detected.

* subversion/bindings/swig/include/proxy.py
   (_retrieve_swig_value): Factor out metadata retrieval from
__getattribute__ to a new function.
   (__getattribute__): Only define __getattribute__ for new style classes.
   (__getattr__): Add back implementation for classic classes.
]]]

Troy

Mime
View raw message