Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python 3 preparation: Handle basestring (Py2) vs. str (Py3) #16064

Closed
wluebbe mannequin opened this issue Apr 7, 2014 · 40 comments
Closed

Python 3 preparation: Handle basestring (Py2) vs. str (Py3) #16064

wluebbe mannequin opened this issue Apr 7, 2014 · 40 comments

Comments

@wluebbe
Copy link
Mannequin

wluebbe mannequin commented Apr 7, 2014

From the Python 2 documentation basestring():
This abstract type is the superclass for str and unicode. It cannot be called or instantiated, but it can be used to test whether an object is an instance of str or unicode. isinstance(obj, basestring) is equivalent to isinstance(obj, (str, unicode)).
New in version 2.3.
This is not available in Python 3.

The tool 2to3 changes basestring into str (the only string type in Py3).

There are 27 effected modules.

This ticket is tracked as a dependency of meta-ticket ticket:16052.

Depends on #18492

Component: distribution

Author: André Apitzsch

Branch/Commit: 7e60f88

Reviewer: Ralf Stephan, Wilfried Luebbe

Issue created by migration from https://trac.sagemath.org/ticket/16064

@wluebbe wluebbe mannequin added this to the sage-6.2 milestone Apr 7, 2014
@wluebbe wluebbe mannequin added c: distribution labels Apr 7, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.2, sage-6.3 May 6, 2014
@a-andre
Copy link

a-andre commented May 18, 2014

Author: André Apitzsch

@a-andre
Copy link

a-andre commented May 18, 2014

Branch: u/aapitzsch/ticket/16064

@a-andre
Copy link

a-andre commented May 18, 2014

Commit: e1252a4

@a-andre
Copy link

a-andre commented May 18, 2014

New commits:

e1252a4replace basestring by str

@rwst
Copy link

rwst commented May 21, 2014

comment:3

Eyeballed and patchbotted ok. Just a naive question: wouldn't the ... str = basestring snippet be needed in every affected file?

@rwst
Copy link

rwst commented May 21, 2014

Reviewer: Ralf Stephan

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented May 21, 2014

comment:5

For unicode strings in Python 2.7 the change will not give the same result as before. See

Python 2.7.6 (default, Mar 22 2014, 22:59:56) 
[GCC 4.8.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> s ='a str'
>>> su = u'a u-str'
>>> isinstance(s, basestring)
True
>>> isinstance(su, basestring)
True
>>> isinstance(s, str)
True
>>> isinstance(su, str)
False
>>> 

As we want a single Python 2 / 3 code base I think this is not the right solution.

@wluebbe wluebbe mannequin added s: needs work and removed s: positive review labels May 21, 2014
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 7, 2014

Changed commit from e1252a4 to 7fc09f9

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 7, 2014

Branch pushed to git repo; I updated commit sha1. New commits:

f4ce4d1replace basestring by str if python version >= 3
7fc09f9Merge remote-tracking branch 'origin/develop' into py3_basestring

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 19, 2014

Branch pushed to git repo; I updated commit sha1. New commits:

63eaffdMerge remote-tracking branch 'origin/develop' into py3_basestring

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 19, 2014

Changed commit from 7fc09f9 to 63eaffd

@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.3, sage-6.4 Aug 10, 2014
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 13, 2014

Branch pushed to git repo; I updated commit sha1. New commits:

3df268bMerge remote-tracking branch 'origin/develop' into py3_basestring

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 13, 2014

Changed commit from 63eaffd to 3df268b

@jdemeyer
Copy link

comment:11

I have a general question about Python3 tickets: would it not be better to simply use the 2to3 tool at build time (i.e. when doing sage -b)? This would fix not only this ticket, but also many others and there would be no need to uglify the code like in this patch.

@a-andre
Copy link

a-andre commented May 25, 2015

comment:18

Made use of six.

I haven't added six as a dependency because that's done in #18492.

@a-andre
Copy link

a-andre commented May 25, 2015

Changed commit from bc70272 to none

@a-andre
Copy link

a-andre commented May 25, 2015

Changed branch from u/aapitzsch/ticket/16064_2 to u/aapitzsch/16064_2

@a-andre
Copy link

a-andre commented May 25, 2015

Commit: 002892e

@jdemeyer
Copy link

Dependencies: #18492

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Jun 9, 2015

comment:21

The patch looks fine. Testing is OK.

But in sage-6.8.beta3 there are still basestrings in sage/quivers/algebra.py line 277 and in sage/quivers/path_semigroup.py lines 340 / 348. The are NOT present sage-6.8.beta0 on which the patch is based. I haven't found out why.

Can you base the patch on sage-6.8.beta3 and change the 3 remaining basestrings. Then I would set the ticket to positive review.

@wluebbe wluebbe mannequin added s: needs work and removed s: needs review labels Jun 9, 2015
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 9, 2015

Changed commit from 002892e to b16df45

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 9, 2015

Branch pushed to git repo; I updated commit sha1. New commits:

eaa8dc7Merge remote-tracking branch 'origin/develop' into py3_basestring_six
b16df45replace remaining basestring by six.string_types

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Jun 9, 2015

comment:24

Right at the beginning of test --all --long (after a fresh make!) I get LOTS of errors like

File "src/sage/combinat/root_system/non_symmetric_macdonald_polynomials.py", line 62, in sage.combinat.root_system.non_symmetric_macdonald_polynomials.NonSymmetricMacdonaldPolynomials
Failed example:
    E = NonSymmetricMacdonaldPolynomials(["A",2,1])
Exception raised:
    Traceback (most recent call last):
      File "/home/wluebbe/sage-6.8.beta3/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 496, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/wluebbe/sage-6.8.beta3/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 858, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.combinat.root_system.non_symmetric_macdonald_polynomials.NonSymmetricMacdonaldPolynomials[0]>", line 1, in <module>
        E = NonSymmetricMacdonaldPolynomials(["A",Integer(2),Integer(1)])
      File "sage/misc/lazy_import.pyx", line 383, in sage.misc.lazy_import.LazyImport.__call__ (build/cythonized/sage/misc/lazy_import.c:3453)
        return self._get_object()(*args, **kwds)
      File "sage/misc/classcall_metaclass.pyx", line 326, in sage.misc.classcall_metaclass.ClasscallMetaclass.__call__ (build/cythonized/sage/misc/classcall_metaclass.c:1204)
        return cls.classcall(cls, *args, **kwds)
      File "/home/wluebbe/sage-6.8.beta3/local/lib/python2.7/site-packages/sage/combinat/root_system/non_symmetric_macdonald_polynomials.py", line 1208, in __classcall__
        q = K(q)
      File "sage/structure/parent.pyx", line 1095, in sage.structure.parent.Parent.__call__ (build/cythonized/sage/structure/parent.c:9523)
        return mor._call_(x)
      File "sage/structure/coerce_maps.pyx", line 109, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4402)
        raise
      File "sage/structure/coerce_maps.pyx", line 104, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4309)
        return C._element_constructor(x)
      File "/home/wluebbe/sage-6.8.beta3/local/lib/python2.7/site-packages/sage/rings/fraction_field.py", line 563, in _element_constructor_
        x = sage_eval(x, self.gens_dict_recursive())
      File "/home/wluebbe/sage-6.8.beta3/local/lib/python2.7/site-packages/sage/misc/sage_eval.py", line 181, in sage_eval
        if not isinstance(source, six.string_types):
    AttributeError: 'module' object has no attribute 'string_types'

Apparently the module six has not yet the attribute string_types. This seems to be the symptom of a circular import. But why after this last, small change?

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Jun 9, 2015

comment:25

Apparently both fraction.field.py and sage-eval.py (see the last 2 listed files) contain import six.
And at the point where the error occurs the import process for module six (started in fraction.field.py) is not yet complete.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 9, 2015

Branch pushed to git repo; I updated commit sha1. New commits:

7e60f88prevent relative import of six

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 9, 2015

Changed commit from b16df45 to 7e60f88

@a-andre
Copy link

a-andre commented Jun 9, 2015

comment:27

There is a new six.py in misc which is relative imported instead of the expected six module.

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Jun 10, 2015

Changed reviewer from Ralf Stephan to Wilfried Luebbe

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Jun 10, 2015

comment:28

I did not think of a new six.py module in src/sage/misc!

All test passed. Looks good.

@rwst
Copy link

rwst commented Jun 10, 2015

comment:29

You were not satisfied with my part of the review, sir?

@rwst
Copy link

rwst commented Jun 10, 2015

Changed reviewer from Wilfried Luebbe to Ralf Stephan, Wilfried Luebbe

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Jun 10, 2015

comment:30

Replying to @rwst:

You were not satisfied with my part of the review, sir?

Sorry about that.

@vbraun
Copy link
Member

vbraun commented Jun 11, 2015

Changed branch from u/aapitzsch/16064_2 to 7e60f88

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants