Opened 3 months ago
Closed 3 months ago
#27275 closed enhancement (fixed)
py3: fix sage.libs.readline.pyx for python3
Reported by:  vklein  Owned by:  

Priority:  major  Milestone:  sage8.7 
Component:  python3  Keywords:  
Cc:  chapoton, embray  Merged in:  
Authors:  Vincent Klein  Reviewers:  Erik Bray, Frédéric Chapoton 
Report Upstream:  N/A  Work issues:  
Branch:  7ccd2a5 (Commits)  Commit:  7ccd2a5546b25c2dc254c392414a6d716ed70eed 
Dependencies:  Stopgaps: 
Description
fix doctests failures : sage t long src/sage/libs/readline.pyx # 10 doctests failed
Change History (28)
comment:1 Changed 3 months ago by
 Branch set to u/vklein/27275
comment:2 Changed 3 months ago by
 Cc chapoton added
 Commit set to aa4cb2a689d3126c1b949d8698a5e9b9031f26ed
 Status changed from new to needs_review
comment:4 Changed 3 months ago by
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.
comment:5 followup: ↓ 7 Changed 3 months ago by
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.
comment:6 Changed 3 months ago by
 Status changed from needs_review to needs_info
comment:7 in reply to: ↑ 5 Changed 3 months ago by
Replying to embray:
Couldn't we instead modify the
copy_text
function to callbytes_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.
comment:8 Changed 3 months ago by
 Commit changed from aa4cb2a689d3126c1b949d8698a5e9b9031f26ed to 689f9a4521eef25e89b732774200210fb958d345
Branch pushed to git repo; I updated commit sha1. New commits:
689f9a4  Trac #27275: wrap copy_text result with a bytes_to_str ...

comment:9 Changed 3 months ago by
 Status changed from needs_info to positive_review
@embray I don't see drawback with your solution, let's try this.
comment:10 Changed 3 months ago by
 Status changed from positive_review to needs_review
comment:11 Changed 3 months ago by
 Reviewers set to Erik Bray
 Status changed from needs_review to positive_review
Just one other minor nit to pick (with the existing tests before this ticket):

src/sage/libs/readline.pyx
diff git a/src/sage/libs/readline.pyx b/src/sage/libs/readline.pyx index e3e7671..43279a7 100644
a b line is removed:: 20 20 21 21 sage: with interleaved_output(): 22 22 ....: print('output') 23 ....: print('current line: ' + repr(copy_text(0, get_end()))) 23 ....: print('current line: ' + 24 ....: repr(copy_text(0, get_end()))) 24 25 ....: print('cursor position: {}'.format(get_point())) 25 26 output 26 27 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.
comment:12 followup: ↓ 13 Changed 3 months ago by
Similarly the line print('cursor position: {}'.format(get_point()))
could just be written simply
print('cursor position:', get_point())
comment:13 in reply to: ↑ 12 Changed 3 months ago by
Replying to embray:
Similarly the line
print('cursor position: {}'.format(get_point()))
could just be written simplyprint('cursor position:', get_point())
Oh thanks ! That's good to know.
I will make these little modifications.
comment:14 Changed 3 months ago by
 Commit changed from 689f9a4521eef25e89b732774200210fb958d345 to aadc9c58cc34eb20b02f3a75194f661ed1f4a4dc
 Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
aadc9c5  Trac #27275: Some syntax enhancement.

comment:15 followups: ↓ 16 ↓ 17 Changed 3 months ago by
Did you test this? When you do print(a, b)
it automatically inserts spaces, so I think now you have an extra space.
comment:16 in reply to: ↑ 15 ; followup: ↓ 19 Changed 3 months ago by
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.
comment:17 in reply to: ↑ 15 Changed 3 months ago by
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.
comment:18 followup: ↓ 20 Changed 3 months ago by
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.
comment:19 in reply to: ↑ 16 Changed 3 months ago by
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('a','b') ('a', 'b')versus Python3:
>>> print('a', 'b') a bSo 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.
comment:20 in reply to: ↑ 18 Changed 3 months ago by
Replying to vklein:
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: 3still pass.
And for python3 the doctest framework doesn't seem to care if there is one or two spaces between
cursor position:
and3
.
I'm not sure why it is because that's wrong.
For the doctests we are using the Python 3 print() function where possible.
comment:21 followup: ↓ 23 Changed 3 months ago by
See e.g. #23551.
comment:22 Changed 3 months ago by
 Commit changed from aadc9c58cc34eb20b02f3a75194f661ed1f4a4dc to 689f9a4521eef25e89b732774200210fb958d345
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
comment:23 in reply to: ↑ 21 Changed 3 months ago by
comment:24 Changed 3 months ago by
I think for miscellaneous doctests where it's really beside the point, I think it should be fine, and better to use futureproof 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...
comment:25 Changed 3 months ago by
 Commit changed from 689f9a4521eef25e89b732774200210fb958d345 to 7ccd2a5546b25c2dc254c392414a6d716ed70eed
comment:26 Changed 3 months ago by
Rebased on 8.7.beta4 and fix doctests with print('a', 'b')
syntax with automatically inserted spaces taken into account.
comment:27 Changed 3 months ago by
 Reviewers changed from Erik Bray to Erik Bray, Frédéric Chapoton
 Status changed from needs_review to positive_review
ok, let it be.
comment:28 Changed 3 months ago by
 Branch changed from u/vklein/27275 to 7ccd2a5546b25c2dc254c392414a6d716ed70eed
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Trac #27275: Fix readline.pyx for python3