On Fri, Dec 22, 2017 at 11:11 PM Daniel Shahaf <d.s_at_daniel.shahaf.name>
wrote:
> troycurtisjr_at_apache.org wrote on Sat, 23 Dec 2017 04:43 +0000:
> > Author: troycurtisjr
> > Date: Sat Dec 23 04:43:26 2017
> > New Revision: 1819110
> >
> > URL: http://svn.apache.org/viewvc?rev=1819110&view=rev
> > Log:
> > On branch swig-py3: Replace hasattr check for a method with try-except.
> >
> > * subversion/bindings/swig/include/proxy.swg
> > (_assert_valid_deep): Replace hasattr check for the 'assert_valid'
> method
> > with a try-except block as some class instances can have the method
> but return
> > False to hasattr().
>
> I'm not too familiar with this code; shouldn't we be fixing the original
> problem, of hasattr() wrongly returning False? I assume it predates the
> branch, though?
>
>
I won't lie, I didn't like this change much, since I didn't feel that I
understood exactly *why* it didn't work. I only found info stating that
hasattr effectively did a getattr, but translated the AttributeError into a
boolean. However, obviously *something* else is different. The attribute
is obviously able to be found in some scenarios, but returns false for
hasattr. So far my attempts to reproduce in a small test class haven't
been successful. Perhaps, I should continue to dig into this one to get to
the bottom of what the difference actually is.
> While reviewing this I also noticed that svn_swig_py_convert_ptr() also
> does this hasattr() check — not sure whether it needs to change too? —
> and also has an "x | 0" construct, which seems suspicious (isn't it a
> no-op?).
>
>
I looked back through the logs in an attempt figure out the rationale for
the "x | 0" construct, but it just shows up that way when it was first
added [0]. I agree, it should just be changed to the define. I can't come
up with a reason to 'or' with 0.
> > Modified:
> >
> subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.swg
> >
> > Modified: subversion/branches/swig-py3/subversion/bindings/swig/include/
> > proxy.swg
> > URL:
> >
> http://svn.apache.org/viewvc/subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.swg?rev=1819110&r1=1819109&r2=1819110&view=diff
> >
> ==============================================================================
> > --- subversion/branches/swig-py3/subversion/bindings/swig/include/
> > proxy.swg (original)
> > +++ subversion/branches/swig-py3/subversion/bindings/swig/include/
> > proxy.swg Sat Dec 23 04:43:26 2017
> > @@ -57,8 +57,11 @@
> > _assert_valid_deep(v)
> > # Ensure that the passed in value isn't a type, which could have an
> > # assert_valid attribute, but it can not be called without an
> > instance.
> > - elif type(value) != type and hasattr(value, "assert_valid"):
> > + elif type(value) != type:
> > + try:
> > value.assert_valid()
> > + except AttributeError:
> > + pass
>
> Hmm. Strictly speaking, the equivalent form would be:
>
> try:
> value.assert_valid
> except AttributeError:
> pass
> else:
> value.assert_valid()
>
> since we don't want to mask any AttributeErrors inside assert_valid().
> I'm not sure how careful we are about this distinction, though.
>
>
Good catch!
> > %}
> >
> > /* Default code for all wrapped proxy classes in Python.
> >
> >
>
> Cheers,
>
> Daniel
>
0: svn diff -r855533:855534
^/subversion/branches/python-bindings-improvements/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c_at_855534
Received on 2017-12-23 16:28:17 CET