py3: several string conversion fixes #24223

embray · 2017-11-16T11:44:24Z

This fixes several (hardly exhaustive) string conversions around Sage such that functions and methods that returned str on Python 2 also return str on Python 3 (and the same for functions and methods that take str as arguments) using the new string utilities from #24222.

This also incorporates/replaces the fixes from #23812.

CC: @fchapoton @jdemeyer

Component: python3

Author: Erik Bray

Branch/Commit: e9a582e

Reviewer: Jeroen Demeyer

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

The text was updated successfully, but these errors were encountered:

embray · 2017-11-16T11:45:34Z

comment:1

With this, plus a few other small fixes (including fixes from other existing tickets merged in) I was able to get the Sage REPL up and 1 + 1 working.

sagetrac-git · 2017-11-20T09:19:58Z

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

`34ef6dc`	`Add HAVE_GMPY2 compile-time constant`
`158d984`	`Force absolute import in have_module()`
`7f06e71`	`Fix documentation`
`3ac146c`	`Add a Cython compile-time constant for Python major version.`
`f71fd41`	`Add new string conversion utilities.`
`a5d5219`	`Add support for optional error handling.`
`627d4c7`	`Add new string conversion utilities.`
`25d6c32`	`Numerous string conversions from bytes to str and from str to bytes for Python 3`

sagetrac-git · 2017-11-20T09:19:58Z

Changed commit from 44a5540 to 25d6c32

embray · 2017-11-20T09:21:05Z

comment:3

Rebased on current version of #24222.

jdemeyer · 2017-11-20T19:08:59Z

comment:4

Something is wrong: there are still the files src/sage/misc/string.*

jdemeyer · 2017-11-20T19:14:49Z

comment:5

This is again a ticket with many small independent changes where a "partial positive review" like what I proposed on #24025 would make sense. I didn't like how we handled that, so how should we proceed here?

embray · 2017-11-21T16:58:54Z

comment:6

Replying to @jdemeyer:

Something is wrong: there are still the files src/sage/misc/string.*

Oops--part of the problem here is I'm moving between two different working trees, one for Python 2 and one for Python 3, so it can get confusing. I'll clean this up.

embray · 2017-11-21T16:59:50Z

comment:7

Replying to @jdemeyer:

This is again a ticket with many small independent changes where a "partial positive review" like what I proposed on #24025 would make sense. I didn't like how we handled that, so how should we proceed here?

I'd be fine if you proposed your changes and tossed it back to me as "needs review", rather than going immediately to "positive review".

jdemeyer · 2017-11-22T14:29:26Z

comment:8

It seems strange to me to use locale.getpreferredencoding() for strings which do not depend on the locale. I would propose:

Use locale.getpreferredencoding() for strings which actually depend on the locale.
Use sys.getfilesystemencoding() for strings which actually depend on the file system encoding.
Use UTF-8 otherwise.

sagetrac-git · 2017-12-21T11:14:50Z

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

`64567aa`	`Make type checks explicit`
`a358aea`	`Require a const char*--this is more consistent for use in cases where`
`64a1c2d`	`Use a less unusual character in the tests`
`1bf6709`	`Use r-strings for the docstrings since they contain slashes`
`2bb8396`	`Clean up these declarations:`
`6d73a41`	`Use more specific type declarations here.`
`56f1c02`	`Numerous string conversions from bytes to str and from str to bytes for Python 3`

sagetrac-git · 2017-12-21T11:14:50Z

Changed commit from 25d6c32 to 56f1c02

sagetrac-git · 2017-12-21T11:20:18Z

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

`a285af4`	`Add new string conversion utilities.`
`5b3719a`	`Add support for optional error handling.`
`bf5f893`	`Make type checks explicit`
`1bd27fc`	`Require a const char*--this is more consistent for use in cases where`
`bd2d931`	`Use a less unusual character in the tests`
`b32f000`	`Use r-strings for the docstrings since they contain slashes`
`ccf7bcd`	`Clean up these declarations:`
`dec9f3a`	`Use more specific type declarations here.`
`a9ebae6`	`Numerous string conversions from bytes to str and from str to bytes for Python 3`

sagetrac-git · 2017-12-21T11:20:18Z

Changed commit from 56f1c02 to a9ebae6

jdemeyer · 2017-12-27T19:20:20Z

comment:12

Is this needs_review again?

embray · 2018-01-02T14:37:25Z

comment:13

Yeah, IIRC this was just a bunch of miscellaneous string conversions from some other tickets, but redone using the sage.cpython.string utilities.

fchapoton · 2018-01-03T12:43:51Z

comment:14

3 failing doctests in integer.pyx

embray · 2018-01-05T12:23:54Z

comment:16

Interesting--when I added str_to_bytes I predicted we might later want to allow Python 2 unicode objects to be supported somehow, but put it off because I didn't think there was much code in Sage that already was expected to handle unicode (except in a few places like unicode art, etc.). But the tests that are failing here clearly show that there are some places it's expected to work.

So I see at least three options:

Modify str_to_bytes to treat unicode appropriate on Python 2
Add some new function like unicode_to_str that's analogous to str_to_bytes (I'm not crazy about this)
Just special case unicode on Python 2 where we do want to support it.

I lean towards 1. but I'm open to ideas.

sagetrac-git · 2018-01-28T08:03:28Z

Changed commit from 00f2776 to 7d68d8c

sagetrac-git · 2018-01-28T08:03:28Z

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

`7d68d8c`	`some details of imports`

jdemeyer · 2018-01-29T13:53:09Z

Reviewer: Jeroen Demeyer

embray · 2018-01-30T12:22:46Z

comment:34

This does not work:

-from sage.cpython.string import FS_ENCODING, str_to_bytes
+from sage.cpython.string cimport FS_ENCODING, str_to_bytes

FS_ENCODING is can't be cimported.

sagetrac-git · 2018-01-30T14:11:47Z

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

`80b0d1f`	`fix import`

sagetrac-git · 2018-01-30T14:11:47Z

Changed commit from 7d68d8c to 80b0d1f

fchapoton · 2018-01-30T14:12:10Z

comment:36

import corrected

embray · 2018-01-30T14:38:18Z

Changed branch from public/python3-trac24223 to u/embray/python3/string-fixes

embray · 2018-01-30T14:38:18Z

comment:37

I rebased and squashed some of these changes.

New commits:

`cc214fd`	`Numerous string conversions from bytes to str and from str to bytes for Python 3`
`f7e1153`	`Use str_to_bytes for 'unicode' (which will be str on Python 3 and unicode on Python 2)`
`a4d6fe3`	`A few additional string conversion fixes for sage.libs.ecl`
`d03a8d4`	`py3: fix pickling of integers and other minor bytes/str fixes`
`e9a582e`	`import -> cimport where appropriate`

embray · 2018-01-30T14:38:18Z

Changed commit from 80b0d1f to e9a582e

vbraun · 2018-01-31T21:40:54Z

comment:38

[sagelib-8.2.beta4] Error compiling Cython file:
[sagelib-8.2.beta4] ------------------------------------------------------------
[sagelib-8.2.beta4] ...
[sagelib-8.2.beta4] from sage.rings.finite_rings.finite_field_prime_modn import FiniteField_prime_modn
[sagelib-8.2.beta4] from sage.rings.finite_rings.finite_field_givaro import FiniteField_givaro
[sagelib-8.2.beta4] from sage.rings.finite_rings.finite_field_ntl_gf2e import FiniteField_ntl_gf2e
[sagelib-8.2.beta4] from sage.libs.pari.all import pari
[sagelib-8.2.beta4] from sage.libs.gmp.all cimport *
[sagelib-8.2.beta4] from sage.cpython.string cimport FS_ENCODING, str_to_bytes
[sagelib-8.2.beta4] ^
[sagelib-8.2.beta4] ------------------------------------------------------------
[sagelib-8.2.beta4] 
[sagelib-8.2.beta4] sage/libs/singular/singular.pyx:40:0: 'sage/cpython/string/FS_ENCODING.pxd' not found
[sagelib-8.2.beta4] 
[sagelib-8.2.beta4] Error compiling Cython file:
[sagelib-8.2.beta4] ------------------------------------------------------------
[sagelib-8.2.beta4] ...
[sagelib-8.2.beta4]     lib = SINGULAR_SO
[sagelib-8.2.beta4] 
[sagelib-8.2.beta4]     if not os.path.exists(lib):
[sagelib-8.2.beta4]         raise ImportError("cannot locate Singular library ({})".format(lib))
[sagelib-8.2.beta4] 
[sagelib-8.2.beta4]     lib = str_to_bytes(lib, FS_ENCODING, "surrogateescape")
[sagelib-8.2.beta4]                            ^
[sagelib-8.2.beta4] ------------------------------------------------------------
[sagelib-8.2.beta4] 
[sagelib-8.2.beta4] sage/libs/singular/singular.pyx:778:28: 'FS_ENCODING' is not a constant, variable or function identifier
[sagelib-8.2.beta4] Traceback (most recent call last):
[sagelib-8.2.beta4]   File "/home/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/Cython/Build/Dependencies.py", line 1179, in cythonize_one_helper
[sagelib-8.2.beta4]     return cythonize_one(*m)
[sagelib-8.2.beta4]   File "/home/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/Cython/Build/Dependencies.py", line 1161, in cythonize_one
[sagelib-8.2.beta4]     raise CompileError(None, pyx_file)
[sagelib-8.2.beta4] CompileError: sage/libs/singular/singular.pyx

fchapoton · 2018-02-01T07:12:27Z

comment:39

Volker, did you use the latest branch ?

embray · 2018-02-01T12:25:45Z

comment:40

He didn't. Setting this back to positive review since this otherwise fixes Jeroen's issue.

vbraun · 2018-02-02T12:06:05Z

Changed branch from u/embray/python3/string-fixes to e9a582e

embray added this to the sage-8.1 milestone Nov 16, 2017

embray added c: python3 labels Nov 16, 2017

embray added the s: needs review label Nov 20, 2017

jdemeyer added s: needs work and removed s: needs review labels Nov 20, 2017

embray modified the milestones: sage-8.1, sage-8.2 Dec 12, 2017

embray added s: needs review and removed s: needs work labels Jan 2, 2018

jdemeyer added s: needs work and removed s: needs review labels Jan 5, 2018

jdemeyer added s: positive review and removed s: needs review labels Jan 29, 2018

embray added s: needs work and removed s: positive review labels Jan 30, 2018

fchapoton added s: needs review and removed s: needs work labels Jan 30, 2018

embray added s: positive review and removed s: needs review labels Jan 30, 2018

vbraun added s: needs work and removed s: positive review labels Jan 31, 2018

embray added s: positive review and removed s: needs work labels Feb 1, 2018

vbraun removed the s: positive review label Feb 2, 2018

vbraun closed this as completed in 56c3223 Feb 2, 2018

fchapoton mentioned this issue Jun 27, 2018

py3: let the repr of numbers be a string #23812

Closed

embray mentioned this issue Feb 16, 2018

py3: minor fixes to sage.libs.pynac #24507

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

py3: several string conversion fixes #24223

py3: several string conversion fixes #24223

embray commented Nov 16, 2017

embray commented Nov 16, 2017

sagetrac-git mannequin commented Nov 20, 2017

sagetrac-git mannequin commented Nov 20, 2017

embray commented Nov 20, 2017

jdemeyer commented Nov 20, 2017

jdemeyer commented Nov 20, 2017

embray commented Nov 21, 2017

embray commented Nov 21, 2017

jdemeyer commented Nov 22, 2017

sagetrac-git mannequin commented Dec 21, 2017

sagetrac-git mannequin commented Dec 21, 2017

sagetrac-git mannequin commented Dec 21, 2017

sagetrac-git mannequin commented Dec 21, 2017

jdemeyer commented Dec 27, 2017

embray commented Jan 2, 2018

fchapoton commented Jan 3, 2018

embray commented Jan 5, 2018

sagetrac-git mannequin commented Jan 28, 2018

sagetrac-git mannequin commented Jan 28, 2018

jdemeyer commented Jan 29, 2018

embray commented Jan 30, 2018

sagetrac-git mannequin commented Jan 30, 2018

sagetrac-git mannequin commented Jan 30, 2018

fchapoton commented Jan 30, 2018

embray commented Jan 30, 2018

embray commented Jan 30, 2018

embray commented Jan 30, 2018

vbraun commented Jan 31, 2018

fchapoton commented Feb 1, 2018

embray commented Feb 1, 2018

vbraun commented Feb 2, 2018

py3: several string conversion fixes #24223

py3: several string conversion fixes #24223

Comments

embray commented Nov 16, 2017

embray commented Nov 16, 2017

sagetrac-git mannequin commented Nov 20, 2017

sagetrac-git mannequin commented Nov 20, 2017

embray commented Nov 20, 2017

jdemeyer commented Nov 20, 2017

jdemeyer commented Nov 20, 2017

embray commented Nov 21, 2017

embray commented Nov 21, 2017

jdemeyer commented Nov 22, 2017

sagetrac-git mannequin commented Dec 21, 2017

sagetrac-git mannequin commented Dec 21, 2017

sagetrac-git mannequin commented Dec 21, 2017

sagetrac-git mannequin commented Dec 21, 2017

jdemeyer commented Dec 27, 2017

embray commented Jan 2, 2018

fchapoton commented Jan 3, 2018

embray commented Jan 5, 2018

sagetrac-git mannequin commented Jan 28, 2018

sagetrac-git mannequin commented Jan 28, 2018

jdemeyer commented Jan 29, 2018

embray commented Jan 30, 2018

sagetrac-git mannequin commented Jan 30, 2018

sagetrac-git mannequin commented Jan 30, 2018

fchapoton commented Jan 30, 2018

embray commented Jan 30, 2018

embray commented Jan 30, 2018

embray commented Jan 30, 2018

vbraun commented Jan 31, 2018

fchapoton commented Feb 1, 2018

embray commented Feb 1, 2018

vbraun commented Feb 2, 2018