#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) 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 11 months ago by vklein

  • Branch set to u/vklein/27275

comment:2 Changed 11 months ago by vklein

  • Cc chapoton added
  • Commit set to aa4cb2a689d3126c1b949d8698a5e9b9031f26ed
  • Status changed from new to needs_review

New commits:

aa4cb2aTrac #27275: Fix readline.pyx for python3

comment:3 Changed 11 months ago by chapoton

  • Cc embray added

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

comment:4 Changed 11 months ago by embray

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: Changed 11 months ago by 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.

comment:6 Changed 11 months ago by embray

  • Status changed from needs_review to needs_info

comment:7 in reply to: ↑ 5 Changed 11 months ago by vklein

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.

comment:8 Changed 11 months ago by git

  • Commit changed from aa4cb2a689d3126c1b949d8698a5e9b9031f26ed to 689f9a4521eef25e89b732774200210fb958d345

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

689f9a4Trac #27275: wrap copy_text result with a bytes_to_str ...

comment:9 Changed 11 months ago by vklein

  • Status changed from needs_info to positive_review

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

comment:10 Changed 11 months ago by vklein

  • Status changed from positive_review to needs_review

comment:11 Changed 11 months ago by embray

  • 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:: 
    2020
    2121    sage: with interleaved_output():
    2222    ....:     print('output')
    23     ....:     print('current line: ' + repr(copy_text(0, get_end())))
     23    ....:     print('current line: ' +
     24    ....:            repr(copy_text(0, get_end())))
    2425    ....:     print('cursor position: {}'.format(get_point()))
    2526    output
    2627    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.

Last edited 11 months ago by embray (previous) (diff)

comment:12 follow-up: Changed 11 months ago by embray

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 11 months ago by vklein

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.

comment:14 Changed 11 months ago by git

  • 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:

aadc9c5Trac #27275: Some syntax enhancement.

comment:15 follow-ups: Changed 11 months ago by embray

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: Changed 11 months ago by 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.

comment:17 in reply to: ↑ 15 Changed 11 months ago by vklein

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: Changed 11 months ago by 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: 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.

Last edited 11 months ago by vklein (previous) (diff)

comment:19 in reply to: ↑ 16 Changed 11 months ago by embray

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.

comment:20 in reply to: ↑ 18 Changed 11 months ago by embray

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: 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.

comment:21 follow-up: Changed 11 months ago by embray

See e.g. #23551.

comment:22 Changed 11 months ago by git

  • 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 11 months ago by vklein

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.

comment:24 Changed 11 months ago by embray

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 11 months ago by git

  • Commit changed from 689f9a4521eef25e89b732774200210fb958d345 to 7ccd2a5546b25c2dc254c392414a6d716ed70eed

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

f21acf8Trac #27275: Fix readline.pyx for python3
a291512Trac #27275: wrap copy_text result with a bytes_to_str ...
7ccd2a5Trac #27275: Enhance doctest syntax.

comment:26 Changed 11 months ago by vklein

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

comment:27 Changed 11 months ago by chapoton

  • 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 11 months ago by vbraun

  • Branch changed from u/vklein/27275 to 7ccd2a5546b25c2dc254c392414a6d716ed70eed
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.