Opened 8 years ago
Closed 7 years ago
#16064 closed enhancement (fixed)
Python 3 preparation: Handle basestring (Py2) vs. str (Py3)
Reported by: | wluebbe | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.8 |
Component: | distribution | Keywords: | python3 |
Cc: | Merged in: | ||
Authors: | André Apitzsch | Reviewers: | Ralf Stephan, Wilfried Luebbe |
Report Upstream: | N/A | Work issues: | |
Branch: | 7e60f88 (Commits, GitHub, GitLab) | Commit: | 7e60f88fba8805110e89fa2f7093d61a2d78542d |
Dependencies: | #18492 | Stopgaps: |
Description
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.
Change History (31)
comment:1 Changed 8 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:2 Changed 8 years ago by
- Branch set to u/aapitzsch/ticket/16064
- Commit set to e1252a45810b1dd857e978336f504843b412bf4d
- Status changed from new to needs_review
comment:3 Changed 8 years ago by
- Status changed from needs_review to positive_review
Eyeballed and patchbotted ok. Just a naive question: wouldn't the ... str = basestring
snippet be needed in every affected file?
comment:4 Changed 8 years ago by
- Reviewers set to Ralf Stephan
comment:5 Changed 8 years ago by
- Status changed from positive_review to needs_work
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.
comment:6 Changed 8 years ago by
- Commit changed from e1252a45810b1dd857e978336f504843b412bf4d to 7fc09f93ebc4b0a662440b79b9d734b1e43d0420
comment:7 Changed 8 years ago by
- Status changed from needs_work to needs_review
comment:8 Changed 8 years ago by
- Commit changed from 7fc09f93ebc4b0a662440b79b9d734b1e43d0420 to 63eaffd5133fc3afb87ac12140aa56f800c12abd
Branch pushed to git repo; I updated commit sha1. New commits:
63eaffd | Merge remote-tracking branch 'origin/develop' into py3_basestring
|
comment:9 Changed 8 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:10 Changed 8 years ago by
- Commit changed from 63eaffd5133fc3afb87ac12140aa56f800c12abd to 3df268bee99266bfb2d75c3d22edb653089b9928
Branch pushed to git repo; I updated commit sha1. New commits:
3df268b | Merge remote-tracking branch 'origin/develop' into py3_basestring
|
comment:11 follow-up: ↓ 12 Changed 8 years ago by
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.
comment:12 in reply to: ↑ 11 ; follow-up: ↓ 16 Changed 8 years ago by
You are right using 2to3
would be the better way, but the tool doesn't touch doctests and it will not fix all compatibility issues.
IMO we should change as much as possible (if it is not ugly) upstream to reduce work for 2to3
.
And in the long run we have to change it anyway.
Changes like #16442, #16529 and even #16071 (if we leave the izip
part out) should be fine.
Let's skip this ticket for the moment and maybe replaced it by using 2to3
at build time.
comment:13 Changed 8 years ago by
- Status changed from needs_review to needs_work
Cython files should support basestring
out of the box in Python 3. So no changes are needed to .pyx
files.
comment:14 Changed 8 years ago by
- Commit changed from 3df268bee99266bfb2d75c3d22edb653089b9928 to bc70272c0df69ebee3b8f080866a61ef900e2f37
Branch pushed to git repo; I updated commit sha1. New commits:
bc70272 | revert basestring changes in *.pyx files
|
comment:15 Changed 8 years ago by
- Status changed from needs_work to needs_review
comment:16 in reply to: ↑ 12 Changed 7 years ago by
- Milestone changed from sage-6.4 to sage-pending
- Status changed from needs_review to needs_work
You wrote:
Replying to aapitzsch:
Let's skip this ticket for the moment and maybe replaced it by using
2to3
at build time.
but later set the ticket to needs_review
... So I don't understand, what is the status of this ticket? I'm changing both the milestone to pending
and the status to needs_work
(as there are merge conflicts anyway), please feel free to revert one or both if appropriate.
comment:17 Changed 7 years ago by
- Milestone changed from sage-pending to sage-6.8
See #18492 for a much better approach using six
.
comment:18 Changed 7 years ago by
- Branch changed from u/aapitzsch/ticket/16064 to u/aapitzsch/ticket/16064_2
- Commit bc70272c0df69ebee3b8f080866a61ef900e2f37 deleted
- Status changed from needs_work to needs_review
Made use of six
.
I haven't added six
as a dependency because that's done in #18492.
comment:19 Changed 7 years ago by
- Branch changed from u/aapitzsch/ticket/16064_2 to u/aapitzsch/16064_2
- Commit set to 002892eb3e5ece1d1e1c440831cf03521c36ea66
comment:20 Changed 7 years ago by
- Dependencies set to #18492
comment:21 Changed 7 years ago by
- Status changed from needs_review to needs_work
The patch looks fine. Testing is OK.
But in sage-6.8.beta3
there are still basestring
s 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 basestring
s. Then I would set the ticket to positive review.
comment:22 Changed 7 years ago by
- Commit changed from 002892eb3e5ece1d1e1c440831cf03521c36ea66 to b16df45af9855bad1a2687db25d57e8fa881a8cf
comment:23 Changed 7 years ago by
- Status changed from needs_work to needs_review
comment:24 Changed 7 years ago by
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?
comment:25 Changed 7 years ago by
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.
comment:26 Changed 7 years ago by
- Commit changed from b16df45af9855bad1a2687db25d57e8fa881a8cf to 7e60f88fba8805110e89fa2f7093d61a2d78542d
Branch pushed to git repo; I updated commit sha1. New commits:
7e60f88 | prevent relative import of six
|
comment:27 Changed 7 years ago by
There is a new six.py
in misc which is relative imported instead of the expected six
module.
comment:28 Changed 7 years ago by
- Reviewers changed from Ralf Stephan to Wilfried Luebbe
- Status changed from needs_review to positive_review
I did not think of a new six.py
module in src/sage/misc
!
All test passed. Looks good.
comment:29 follow-up: ↓ 30 Changed 7 years ago by
- Reviewers changed from Wilfried Luebbe to Ralf Stephan, Wilfried Luebbe
You were not satisfied with my part of the review, sir?
comment:30 in reply to: ↑ 29 Changed 7 years ago by
comment:31 Changed 7 years ago by
- Branch changed from u/aapitzsch/16064_2 to 7e60f88fba8805110e89fa2f7093d61a2d78542d
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
replace basestring by str