py3: fix sage.libs.readline.pyx for python3

[vinklein]

fix doctests failures : sage -t --long src/sage/libs/readline.pyx # 10 doctests failed

CC: @fchapoton @embray

Component: python3

Author: Vincent Klein

Branch/Commit: 7ccd2a5

Reviewer: Erik Bray, Frédéric Chapoton

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

[vinklein]

Branch: u/vklein/27275

[vinklein]

Commit: aa4cb2a

[vinklein]

New commits:

aa4cb2a

Trac #27275: Fix readline.pyx for python3

[fchapoton]

comment:3

seems ok at first sight.. Erik, do you approve ?

[embray]

comment:4

I'm not sure. I don't think bytes_to_str should be used in doctests if it can be helped, but that's more of an arbitrary stylistic choice. Let me look at whether there's a nicer way.

[embray]

comment:5

Couldn't we instead modify the copy_text function to call bytes_to_str on the return value? AFAICT this code isn't even used anywhere in Sage, but it would probably be nicer that way regardless.

[vinklein]

comment:7

Replying to @embray:

Couldn't we instead modify the copy_text function to call bytes_to_str on the return value? AFAICT this code isn't even used anywhere in Sage, but it would probably be nicer that way regardless.

I will try that.

[sagetrac-git]

Changed commit from aa4cb2a to 689f9a4

[sagetrac-git]

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

689f9a4

Trac #27275: wrap copy_text result with a bytes_to_str ...

[vinklein]

comment:9

@embray I don't see drawback with your solution, let's try this.

[embray]

comment:11

Just one other minor nit to pick (with the existing tests before this ticket):

diff --git a/src/sage/libs/readline.pyx b/src/sage/libs/readline.pyx
index e3e7671..43279a7 100644
--- a/src/sage/libs/readline.pyx
+++ b/src/sage/libs/readline.pyx
@@ -20,7 +20,8 @@ line is removed::
 
     sage: with interleaved_output():
     ....:     print('output')
-    ....:     print('current line: ' + repr(copy_text(0, get_end())))
+    ....:     print('current line: ' +
+    ....:            repr(copy_text(0, get_end())))
     ....:     print('cursor position: {}'.format(get_point()))
     output
     current line: ''

with the Python 3 print() function (which I believe we are using in most modules on Python 2 as well), there's no need for explicit string concatenation with +. The prints() in this test could be written like print('current line:', blahblahblah). I think we should promote more idiomatic code (even if in relatively minor tests that most users won't look at).

But don't bother unless you feel like it.

[embray]

Reviewer: Erik Bray

[embray]

comment:12

Similarly the line print('cursor position: {}'.format(get_point())) could just be written simply

print('cursor position:', get_point())

[vinklein]

comment:13

Replying to @embray:

Similarly the line print('cursor position: {}'.format(get_point())) could just be written simply

print('cursor position:', get_point())

Oh thanks ! That's good to know.

I will make these little modifications.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

aadc9c5

Trac #27275: Some syntax enhancement.

[sagetrac-git]

Changed commit from 689f9a4 to aadc9c5

[embray]

comment:15

Did you test this? When you do print(a, b) it automatically inserts spaces, so I think now you have an extra space.

[tscrim]

comment:16

Replying to @embray:

Did you test this? When you do print(a, b) it automatically inserts spaces, so I think now you have an extra space.

Be careful with print on Python2:

>>> print('a','b')
('a', 'b')

versus Python3:

>>> print('a', 'b')
a b

So I would not use the comma version in order to be Python2/3 compatible.

[vinklein]

comment:17

Replying to @embray:

Did you test this? When you do print(a, b) it automatically inserts spaces, so I think now you have an extra space.

I think i have. But i am not sure anymore. And i don't understand why the last patchbot is green. I am currently doing more tests.

[vinklein]

comment:18

Well that's pretty strange even if with python2 you get that in sage console:

sage: from sage.libs.readline import *
sage: replace_line('foobar', 0)
sage: set_point(3)
sage: print('cursor position: ', get_point())
('cursor position: ', 3)

The doctest

sage: print('cursor position: ', get_point())
cursor position: 3

still pass.

And for python3 the doctest framework doesn't seem to care if there is one or two spaces between cursor position: and 3.

Even if it pass i think it's more clear and consistent to rollback to the format syntax.

[embray]

comment:19

Replying to @tscrim:

Replying to @embray:

Did you test this? When you do print(a, b) it automatically inserts spaces, so I think now you have an extra space.

Be careful with print on Python2:

>>> print('a','b')
('a', 'b')

versus Python3:

>>> print('a', 'b')
a b

So I would not use the comma version in order to be Python2/3 compatible.

We're using from future import print_function almost everywhere. If there's a module that's not using it that's a mistake.

[embray]

comment:20

Replying to @vinklein:

Well that's pretty strange even if with python2 you get that in sage console:

sage: from sage.libs.readline import *
sage: replace_line('foobar', 0)
sage: set_point(3)
sage: print('cursor position: ', get_point())
('cursor position: ', 3)

The doctest

sage: print('cursor position: ', get_point())
cursor position: 3

still pass.

And for python3 the doctest framework doesn't seem to care if there is one or two spaces between cursor position: and 3.

I'm not sure why it is because that's wrong.

For the doctests we are using the Python 3 print() function where possible.

[embray]

comment:21

See e.g. #23551.

[sagetrac-git]

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

[sagetrac-git]

Changed commit from aadc9c5 to 689f9a4

[vinklein]

comment:23

Replying to @embray:

See e.g. #23551.

Ok that explain things !
But is it a good thing to use the print('a', 'b') syntax right now ?
Travis comment and mines show that it's confusing if you are not aware of #23551.

[embray]

comment:24

I think for miscellaneous doctests where it's really beside the point, I think it should be fine, and better to use future-proof best practices. It's just in tutorials and other educational documentation for beginners where one needs to be a little more careful about this (or better yet, teach the difference!)

I hope we can move toward using the print function in the REPL even on Python 2 before long...

[sagetrac-git]

Changed commit from 689f9a4 to 7ccd2a5

[sagetrac-git]

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

f21acf8	Trac #27275: Fix readline.pyx for python3
a291512	Trac #27275: wrap copy_text result with a bytes_to_str ...
7ccd2a5	Trac #27275: Enhance doctest syntax.

[vinklein]

comment:26

Rebased on 8.7.beta4 and fix doctests with print('a', 'b') syntax with automatically inserted spaces taken into account.

[fchapoton]

comment:27

ok, let it be.

[fchapoton]

Changed reviewer from Erik Bray to Erik Bray, Frédéric Chapoton

[vbraun]

Changed branch from u/vklein/27275 to 7ccd2a5