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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Daniel Shahaf-2
Good morning Troy,

[hidden email] 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.

> Modified: subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.py
> URL: http://svn.apache.org/viewvc/subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.py?rev=1818995&r1=1818994&r2=1818995&view=diff
> ==============================================================================
> --- subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.py (original)
> +++ subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.py Fri Dec 22 03:52:59 2017
> @@ -12,17 +12,36 @@
>      if "_is_valid" in self.__dict__:
>        assert self.__dict__["_is_valid"](), "Variable has already been deleted"
>  
> -  def __getattr__(self, name):
> -    """Get an attribute from this object"""
> -    self.assert_valid()
> +  def __getattribute__(self, name):
> +    """Manage access to all attributes of this object."""
>  
> -    value = _swig_getattr(self, self.__class__, name)
> +    # Start by mimicing __getattr__ behavior: immediately return __dict__ or
> +    # items directly present in __dict__
> +    mydict = object.__getattribute__(self, '__dict__')
> +    if name == "__dict__":
> +      return mydict
> +
> +    if name in mydict:
> +      return mydict[name]
> +
> +    object.__getattribute__(self, 'assert_valid')()
> +
> +    try:
> +      value = object.__getattribute__(self, name)
> +    except AttributeError:
> +      value = _swig_getattr(self,
> +                            object.__getattribute__(self, '__class__'),
> +                            name)
>  
>      # If we got back a different object than we have, we need to copy all our
>      # metadata into it, so that it looks identical
> -    members = self.__dict__.get("_members")
> -    if members is not None:
> -      _copy_metadata_deep(value, members.get(name))
> +    try:
> +      members = object.__getattribute__(self, '_members')
> +      if name in members:
> +          _copy_metadata_deep(value, members[name])
> +          # Verify that the new object is good
> +    except AttributeError:
> +      pass
>  
>      # Verify that the new object is good
>      _assert_valid_deep(value)

Cheers,

Daniel

Reply | Threaded
Open this post in threaded view
|

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

Troy Curtis Jr


On Fri, Dec 29, 2017 at 11:46 PM Daniel Shahaf <[hidden email]> wrote:
Good morning Troy,

[hidden email] 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'. 

Just the move to the new style classes did not require the move from __getattr__ to __getattribute__, but rather the swig implementation together with the pool/metadata management requirements of the subversion interfaces did.  For old style classes, swig uses __getattr__ together with a private set of attributes holding getter and setter methods to provide the natural attribute access pattern of 'foo.bar', but without 'bar' actually existing as an attribute.  This way swig can correctly forward to underlying getter/setter functions of the wrapped code.  The subversion bindings then make use of this by overriding __setattr__ and __getattr__ to more perfectly construct returned python objects.  First, it saves off the original object in __setattr__ (which also saves it, along with the associated pool, from getting garbage collected).  Then in __getattr__ it uses attributes from the original object to fill out metadata on the newly constructed object returned from the underlying swig getter methods.

When '-classic' is not used in SWIG, and you are on anything newer than Python 2.2, you get new style classes.  A really nice feature of new style classes are descriptors, or properties.  This allows easy specification of getter/setter code for individual attributes built-in to the class syntax [2].  As this is exactly what swig is trying to accomplish, it uses descriptors where available (in the generated code you can see all the 'if _newclass' constructs accomplishing this).  However, by doing this, __getattr__ is now no longer called for any of those attributes, which breaks the pool/metadata management code! 

The alternative is __getattribute__, which gets *every* attribute access, unlike __getattr__ which is only invoked when all standard lookup fails.  That gets us what we need for the pool management code, but it means there is additional responsibility to mimic the standard behavior as well.  I know from personal experience how easy it is to get into infinite recursion loops. ;)  Mimicing the standard attribute lookup behavior is the rationale behind first resolving from __dict__, then using object.__getattribute__, then using swig's interface.  Without the initial __dict__ lookup, you get into infinite recursion trying to lookup the other basic attributes.  Next, object.__getattribute__ does a standard lookup, and finally the swig lookup picks up any internal swig specific values that aren't accessible by standard means.  Now, with a reference to the returned value, the existing pool/metadata code can be applied properly.

On the compatibility front, the notable change is the move to new style classes.  For code using instances of the objects there should be no functional change. The __getattr__/__getattribute__ using swig-magic-methods/descriptors are only implementation details.  If for some reason users of the bindings inherited from those classes, then there might be a noticeable change in the move, especially if they try overloading internal features. I assume that in doing so, you are already asking for trouble.  It would also make those derived classes go from classic to new style, but the distinction between the classic vs new classes really come down to:
  1. Method resolution order (only change relative to multiple inheritance),
  2. Instances of the class have a real type() and isn't always 'instance' like old-style classes
  3. You can use descriptors/properties

Which I would say is fairly safe compatibility wise, since again this would only impact users that subclassed these generated classes, and tried to dig into internal implementation details. This seems fairly unlikely (since all the important bits have leading underscores indicating internal use only).  Though I suppose that without documentation stating that you *shouldn't* derive from the generated swig classes, there is always the possibility that users could have done so, and thus be surprised at suddenly having new-style classes.

On a final note, I believe that we could use a conditional on _newclass to use __getattr__ and __getattribute__ (and return the -classic option in the python 2 swig invocation), and as long as we called into an appropriately common function, the code would not be particularly repetitive.  However, I thought this unnecessarily complicated the code and reduced commonality between py2 and py3.  If you think the compatibility concerns are too big, then I could explore that conditional implementation.

Hope this helps, and isn't too long winded. ;)

Troy




> Modified: subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.py
> URL: http://svn.apache.org/viewvc/subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.py?rev=1818995&r1=1818994&r2=1818995&view=diff
> ==============================================================================
> --- subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.py (original)
> +++ subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.py Fri Dec 22 03:52:59 2017
> @@ -12,17 +12,36 @@
>      if "_is_valid" in self.__dict__:
>        assert self.__dict__["_is_valid"](), "Variable has already been deleted"
>
> -  def __getattr__(self, name):
> -    """Get an attribute from this object"""
> -    self.assert_valid()
> +  def __getattribute__(self, name):
> +    """Manage access to all attributes of this object."""
>
> -    value = _swig_getattr(self, self.__class__, name)
> +    # Start by mimicing __getattr__ behavior: immediately return __dict__ or
> +    # items directly present in __dict__
> +    mydict = object.__getattribute__(self, '__dict__')
> +    if name == "__dict__":
> +      return mydict
> +
> +    if name in mydict:
> +      return mydict[name]
> +
> +    object.__getattribute__(self, 'assert_valid')()
> +
> +    try:
> +      value = object.__getattribute__(self, name)
> +    except AttributeError:
> +      value = _swig_getattr(self,
> +                            object.__getattribute__(self, '__class__'),
> +                            name)
>
>      # If we got back a different object than we have, we need to copy all our
>      # metadata into it, so that it looks identical
> -    members = self.__dict__.get("_members")
> -    if members is not None:
> -      _copy_metadata_deep(value, members.get(name))
> +    try:
> +      members = object.__getattribute__(self, '_members')
> +      if name in members:
> +          _copy_metadata_deep(value, members[name])
> +          # Verify that the new object is good
> +    except AttributeError:
> +      pass
>
>      # Verify that the new object is good
>      _assert_valid_deep(value)

Cheers,

Daniel

Reply | Threaded
Open this post in threaded view
|

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

Branko Čibej
On 30.12.2017 16:11, Troy Curtis Jr wrote:

>
>
> On Fri, Dec 29, 2017 at 11:46 PM Daniel Shahaf <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     Good morning Troy,
>
>     [hidden email] <mailto:[hidden email]> 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.

-- Brane
Reply | Threaded
Open this post in threaded view
|

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

Troy Curtis Jr


On Sat, Dec 30, 2017 at 12:49 PM Branko Čibej <[hidden email]> wrote:
On 30.12.2017 16:11, Troy Curtis Jr wrote:
>
>
> On Fri, Dec 29, 2017 at 11:46 PM Daniel Shahaf <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     Good morning Troy,
>
>     [hidden email] <mailto:[hidden email]> 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

py2-classic-classes-swig.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Branko Čibej
On 31.12.2017 03:05, Troy Curtis Jr wrote:

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

[...]

> +· # SWIG classes generated with -classic do not define this variable,
> +· # so set it to 0 when it doesn't exist
> +· try: _newclass
> +· except NameError: _newclass = 0

I prefer to break the try/except blocks onto separate lines, and to use
None for the tristate idiom value:

    try:
      _newclass
    except NameError:
      _newclass = None


-- Brane

Reply | Threaded
Open this post in threaded view
|

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

Troy Curtis Jr


On Tue, Jan 2, 2018 at 3:12 AM Branko Čibej <[hidden email]> wrote:
On 31.12.2017 03:05, Troy Curtis Jr wrote:
>
>     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

[...]

> +· # SWIG classes generated with -classic do not define this variable,
> +· # so set it to 0 when it doesn't exist
> +· try: _newclass
> +· except NameError: _newclass = 0

I prefer to break the try/except blocks onto separate lines, and to use
None for the tristate idiom value:

    try:
      _newclass
    except NameError:
      _newclass = None


Using None here is certainly more Pythonic, but in this case I was trying to match up with what swig generates:

 try:
   _object = object
   _newclass = 1
 except __builtin__.Exception:
   class _object:
     pass
   _newclass = 0

In this case we only need the _newclass variable defined, and not the "empty" class definition.  In all the conditional cases which use that value, either way should work, but I think it is likely better to stick with 0 for consistency in this case.

However, I can understand the formatting request.

Other than that, is the consensus that we should continue with classic classes in Python 2 with the conditional logic, or use a common implementation for the python2/python3 code like is currently in the swig-py3 branch?

Troy
Reply | Threaded
Open this post in threaded view
|

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

Branko Čibej
On 04.01.2018 02:48, Troy Curtis Jr wrote:

>
>
> On Tue, Jan 2, 2018 at 3:12 AM Branko Čibej <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     On 31.12.2017 03:05, Troy Curtis Jr wrote:
>     >
>     >     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
>
>     [...]
>
>     > +· # SWIG classes generated with -classic do not define this
>     variable,
>     > +· # so set it to 0 when it doesn't exist
>     > +· try: _newclass
>     > +· except NameError: _newclass = 0
>
>     I prefer to break the try/except blocks onto separate lines, and
>     to use
>     None for the tristate idiom value:
>
>         try:
>           _newclass
>         except NameError:
>           _newclass = None
>
>
> Using None here is certainly more Pythonic, but in this case I was
> trying to match up with what swig generates:
>
>  try:
>    _object = object
>    _newclass = 1
>  except __builtin__.Exception:
>    class _object:
>      pass
>    _newclass = 0
>
> In this case we only need the _newclass variable defined, and not the
> "empty" class definition.  In all the conditional cases which use that
> value, either way should work, but I think it is likely better to
> stick with 0 for consistency in this case.
>
> However, I can understand the formatting request.
>
> Other than that, is the consensus that we should continue with classic
> classes in Python 2 with the conditional logic, or use a common
> implementation for the python2/python3 code like is currently in the
> swig-py3 branch?

The differences between old-style and new-style classes is tricky.
Offhand I can think of differences in invocation of base class
constructors, for example. I'd say leave old style classes for Python 2
precisely because the change would not be 100% backwards compatible.

-- Brane