Opened 5 years ago

Closed 5 years ago

#20814 closed enhancement (fixed)

py3 print : not tested cases, step 5

Reported by: chapoton Owned by:
Priority: major Milestone: sage-7.3
Component: python3 Keywords:
Cc: tscrim Merged in:
Authors: Frédéric Chapoton Reviewers: Jori Mäntysalo, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: fc51d8e (Commits, GitHub, GitLab) Commit: fc51d8e01998efca5901063886723ff0437f2bd7
Dependencies: Stopgaps:

Status badges

Description (last modified by chapoton)

another small step towards python3

some more print converted to use python3 syntax

Change History (27)

comment:1 Changed 5 years ago by chapoton

  • Branch set to public/20814
  • Commit set to fd1af37f1c6f4d520f86b6ae86ccc57234380180
  • Status changed from new to needs_review

New commits:

5ef5316more print converted to python3 in py files
fd1af37more print converted to py3 in .py files

comment:2 Changed 5 years ago by jmantysalo

  • Reviewers set to Jori Mäntysalo
  • Status changed from needs_review to needs_work

An error here:

-    if verbose: print "Check:",
+    if verbose:
+        print("Check:", end="")

needs a space after colon.

comment:3 Changed 5 years ago by jmantysalo

I found no other errors. You can mark this as positive_review after correcting above error.

This is probably my last review for a week. Btw, if you have much time you might be interested in reviewing the code at #20769, or if you have some minutes, making a comment to #20669.

comment:4 Changed 5 years ago by git

  • Commit changed from fd1af37f1c6f4d520f86b6ae86ccc57234380180 to 93e0bb5e166e2c2fa97a1744cedf54af46ba74e4

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

93e0bb5trac 20814 detail

comment:5 Changed 5 years ago by jmantysalo

  • Status changed from needs_work to positive_review

comment:6 Changed 5 years ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict, wait for next beta

comment:7 Changed 5 years ago by git

  • Commit changed from 93e0bb5e166e2c2fa97a1744cedf54af46ba74e4 to 08967ef6b3631a025884b89261ce517d056c1403

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

08967efMerge branch 'public/20814' into 7.3.b4

comment:8 Changed 5 years ago by chapoton

  • Status changed from needs_work to needs_review

comment:9 Changed 5 years ago by chapoton

  • Status changed from needs_review to needs_work

comment:10 Changed 5 years ago by git

  • Commit changed from 08967ef6b3631a025884b89261ce517d056c1403 to a7146ea37f83bf67e1c2be6294018bf6586f359f

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

a7146eacorrect or disactivate the last python2-only print doctests

comment:11 Changed 5 years ago by chapoton

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:12 Changed 5 years ago by jmantysalo

  • Cc tscrim added

No time now. Travis?

comment:13 Changed 5 years ago by chapoton

bot is green

comment:14 follow-up: Changed 5 years ago by tscrim

So with this, every time from this point forward we can never have a Python2 print statement in the doctests? Do I understand the ticket description correctly?

comment:15 in reply to: ↑ 14 Changed 5 years ago by chapoton

Replying to tscrim:

So with this, every time from this point forward we can never have a Python2 print statement in the doctests? Do I understand the ticket description correctly?

Yes, I think so. this seems to be triggered by the change in structure/test_factory.py. I was not expecting that, but as this was something I wanted to do anyway, I am happy to do it here.

1) I have not tested the new behaviour by adding a fake file with a bad print 2) one should also check that the print behaviour in console and notebooks is still py2 3) maybe this should be discussed on sage-devel, though this is not a major change really 4) maybe this can/should be split in another ticket ?

This is the keystone of all my previous changes in the doc, that prevents any regression.

Version 0, edited 5 years ago by chapoton (next)

comment:16 Changed 5 years ago by chapoton

I have tested by adding a fake new file with a bad print in the doc: tests do not pass

I have checked that the behaviour of print is unchanged in console and both notebooks.

I still do not understand why this desirable behaviour happens.

comment:17 Changed 5 years ago by chapoton

ping ?

comment:18 Changed 5 years ago by tscrim

I think it was the change in misc/dev_tools.py. While in principle, I am for this change, I do believe we should have some discussion on sage-devel about it. In general, we can write 2/3 compatible docstrings, but we cannot strictly enforce it. I am in favor of enforcing the Python3 syntax, but it does create a discrepancy (at least if we add a strictly Python3 print statement). If there are no objections on sage-devel, then I am content with putting this to a positive review.

comment:19 Changed 5 years ago by chapoton

I do not think that it comes from the changes in misc/dev_tools.py

I have really tried to see where the change of behaviour came from, with no success so far.

I would now prefer this ticket not to make this change of behaviour, but for this I need to understand what has to be undone, and I have not clue.

comment:20 Changed 5 years ago by git

  • Commit changed from a7146ea37f83bf67e1c2be6294018bf6586f359f to d832810a08c0978cabaefd2bd86c310bb79ac993

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

d832810trac 20814 restore the exact file where future import is not yet welcome

comment:21 Changed 5 years ago by chapoton

  • Description modified (diff)

ok, I think I have found that the problems were coming from the future import in src/sage/modular/etaproducts.py

No idea what happens exactly, but undoing this change gives us back the usual behaviour.

So this is back to being a very simple print ticket, one of the last ones for py files.

comment:22 Changed 5 years ago by tscrim

  • Reviewers changed from Jori Mäntysalo to Jori Mäntysalo, Travis Scrimshaw

If you could remove the added space in the copyright header for consistency, then you can set a positive review on my behalf.

comment:23 Changed 5 years ago by chapoton

The added space remove I can (Yoda mode), but this change is suggested by pep8. Should I remove ?

comment:24 Changed 5 years ago by tscrim

I think so. We consistently break the PEP8 in this case, and in our doc about file headers, the copyright is given without a space as well.

comment:25 Changed 5 years ago by git

  • Commit changed from d832810a08c0978cabaefd2bd86c310bb79ac993 to fc51d8e01998efca5901063886723ff0437f2bd7

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

fc51d8etarc 20814 not pep8 for copyright header

comment:26 Changed 5 years ago by tscrim

  • Status changed from needs_review to positive_review

Thanks.

comment:27 Changed 5 years ago by vbraun

  • Branch changed from public/20814 to fc51d8e01998efca5901063886723ff0437f2bd7
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.