Opened 2 years ago
Closed 2 years ago
#27275 closed enhancement (fixed)
py3: fix sage.libs.readline.pyx for python3
Reported by: | vklein | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.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, GitHub, GitLab) | 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 2 years ago by
- Branch set to u/vklein/27275
comment:2 Changed 2 years ago by
- Cc chapoton added
- Commit set to aa4cb2a689d3126c1b949d8698a5e9b9031f26ed
- Status changed from new to needs_review
comment:4 Changed 2 years 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 follow-up: ↓ 7 Changed 2 years 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 2 years ago by
- Status changed from needs_review to needs_info
comment:7 in reply to: ↑ 5 Changed 2 years 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 2 years 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 2 years 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 2 years ago by
- Status changed from positive_review to needs_review
comment:11 Changed 2 years 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 follow-up: ↓ 13 Changed 2 years 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 2 years 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 2 years 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 follow-ups: ↓ 16 ↓ 17 Changed 2 years 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 ; follow-up: ↓ 19 Changed 2 years 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 2 years 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 follow-up: ↓ 20 Changed 2 years 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 2 years 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 2 years 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 follow-up: ↓ 23 Changed 2 years ago by
See e.g. #23551.
comment:22 Changed 2 years 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 2 years ago by
comment:24 Changed 2 years ago by
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...
comment:25 Changed 2 years ago by
- Commit changed from 689f9a4521eef25e89b732774200210fb958d345 to 7ccd2a5546b25c2dc254c392414a6d716ed70eed
comment:26 Changed 2 years 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 2 years 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 2 years 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