Opened 7 months ago

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

  • Branch set to u/vklein/27275

comment:2 Changed 7 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 7 months ago by chapoton

  • Cc embray added

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

comment:4 Changed 7 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 7 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 7 months ago by embray

  • Status changed from needs_review to needs_info

comment:7 in reply to: ↑ 5 Changed 7 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 7 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 7 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 7 months ago by vklein

  • Status changed from positive_review to needs_review

comment:11 Changed 7 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 7 months ago by embray (previous) (diff)

comment:12 follow-up: Changed 7 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 7 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 7 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 7 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 7 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 7 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 7 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 7 months ago by vklein (previous) (diff)

comment:19 in reply to: ↑ 16 Changed 7 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 7 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 7 months ago by embray

See e.g. #23551.

comment:22 Changed 7 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 7 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 7 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 7 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 7 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 7 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 7 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.