On Sat, Dec 30, 2017 at 12:49 PM Branko Čibej <brane_at_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_at_daniel.shahaf.name
> > <mailto:d.s_at_daniel.shahaf.name>> wrote:
> >
> > Good morning Troy,
> >
> > troycurtisjr_at_apache.org <mailto:troycurtisjr_at_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
Received on 2017-12-31 03:05:48 CET