You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Branko Čibej <br...@apache.org> on 2018/01/02 09:12:48 UTC
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
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
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
Posted by Branko Čibej <br...@apache.org>.
On 04.01.2018 02:48, Troy Curtis Jr wrote:
>
>
> On Tue, Jan 2, 2018 at 3:12 AM Branko Čibej <brane@apache.org
> <ma...@apache.org>> 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
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
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Tue, Jan 2, 2018 at 3:12 AM Branko Čibej <br...@apache.org> 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