On Sun, Feb 11, 2018 at 2:38 AM Daniel Shahaf <d.s_at_daniel.shahaf.name>
wrote:
> Troy Curtis Jr wrote on Sun, 11 Feb 2018 02:28 +0000:
> > I committed the fix to the bindings in
> > https://svn.apache.org/viewvc?view=revision&revision=1823802 . In
> addition
> > to Kenneth's suggestion of opening in binary mode, I switched the imports
> > so that the python2-future's implementation would not get inadvertently
> > pulled in. Everything looked fine with the how python2-future's open
> > worked (since it did in fact use the Python 3's open() semantics), but I
> > think it best that on the intended modules are included. I also added a
> > test which duplicated the issue (with python2-future installed at least),
> > and confirms the fix.
> >
> > This is a relatively isolated change, but fixes surprising behavior (as
> > Kenneth can attest to), does something like this make sense to propose
> for
> > the 1.10 branch?
>
> The change is non-invasive, backwards-compatible, fixes a user-visible
> bug, is unlikely to introduce new ones, and has a test. That makes it a
> good backport candidate.
>
> Usually we have just two 'live' branches in addition to trunk, but
> currently
> we have three (1.8 under security support, 1.9 supported, 1.10 in beta), so
> you might consider nominating this to 1.9 in addition to 1.10.
>
> That said, I'm a bit puzzled at changing the order of imports. Our
> practise throughout the codebase appears to be 'try: import py3name;
> except ImportError: import py2name', and I don't understand why this one
> case should be different. Secondly, if I understand correctly, the
> 'past' package makes 'import __builtins__' work on py3, so changing the
> order of imports simply trades one potential problem for another,
> doesn't it? And thirdly, why would it be a problem if 'from builtins
> import
> open' worked under py2, so long as it had the same semantics as it
> does under py3? (I.e., so long as it set locals()['open'] to a function
> with
> the same semantics as py3's open() function)
>
>
Not only is that convention, but with Kenneth's suggestion, the open()
works correctly, with or without the python future package installed (in my
testing at least). So it should work fine either way.
For me it was more of a semantics thing. The intention of the original
block was to import builtins when running in Python 3 (which would not have
actually worked anyway, since this module is obviously not actually ready
to run under Python 3), and __builtin__ in Python 2. But on some systems,
those with python2-future installed, it actually imports a separate
'builtins' package even in Python 2. You could make the case that as long
as the end result works, it doesn't really matter, even if that wasn't the
intention of the code. In that case the prudent thing to do would be to
add a test environment with python2-future installed and a different one
without. Admittedly, for this extremely limited usage, adding a new
"dedicated" builder is likely way overkill.
I hadn't noticed the 'past' package. Poking around at it though it looks
like the Python 2 behavior is tucked behind a 'past.builtins' module:
/usr/lib/python3.6/site-packages/past/builtins
So I don't think there will be an issue with the import __builtin__ pulling
in a separate, external, module even when the python future package is
installed.
Of course, the "real fix" is to make all the modules Py2/Py3 compatible,
then the import of Py3's open() semantics is perfectly acceptable and
expected throughout the module ;)
All this being said, I'm fine reverting the import swap if that is what is
desired, as all indications are that it works either way now. I simply
liked the communication of intent better with this order.
Troy
Received on 2018-02-11 15:28:12 CET