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:  sage7.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: 
Description (last modified by )
another small step towards python3
some more print converted to use python3 syntax
Change History (27)
comment:1 Changed 5 years ago by
 Branch set to public/20814
 Commit set to fd1af37f1c6f4d520f86b6ae86ccc57234380180
 Status changed from new to needs_review
comment:2 Changed 5 years ago by
 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
comment:4 Changed 5 years ago by
 Commit changed from fd1af37f1c6f4d520f86b6ae86ccc57234380180 to 93e0bb5e166e2c2fa97a1744cedf54af46ba74e4
Branch pushed to git repo; I updated commit sha1. New commits:
93e0bb5  trac 20814 detail

comment:5 Changed 5 years ago by
 Status changed from needs_work to positive_review
comment:6 Changed 5 years ago by
 Status changed from positive_review to needs_work
Merge conflict, wait for next beta
comment:7 Changed 5 years ago by
 Commit changed from 93e0bb5e166e2c2fa97a1744cedf54af46ba74e4 to 08967ef6b3631a025884b89261ce517d056c1403
Branch pushed to git repo; I updated commit sha1. New commits:
08967ef  Merge branch 'public/20814' into 7.3.b4

comment:8 Changed 5 years ago by
 Status changed from needs_work to needs_review
comment:9 Changed 5 years ago by
 Status changed from needs_review to needs_work
comment:10 Changed 5 years ago by
 Commit changed from 08967ef6b3631a025884b89261ce517d056c1403 to a7146ea37f83bf67e1c2be6294018bf6586f359f
Branch pushed to git repo; I updated commit sha1. New commits:
a7146ea  correct or disactivate the last python2only print doctests

comment:11 Changed 5 years ago by
 Description modified (diff)
 Status changed from needs_work to needs_review
comment:13 Changed 5 years ago by
bot is green
comment:14 followup: ↓ 15 Changed 5 years ago by
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
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.
EDIT: I do not understand by what change this is triggered. Maybe one in misc ?
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 sagedevel, 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.
comment:16 Changed 5 years ago by
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
ping ?
comment:18 Changed 5 years ago by
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 sagedevel 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 sagedevel, then I am content with putting this to a positive review.
comment:19 Changed 5 years ago by
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
 Commit changed from a7146ea37f83bf67e1c2be6294018bf6586f359f to d832810a08c0978cabaefd2bd86c310bb79ac993
Branch pushed to git repo; I updated commit sha1. New commits:
d832810  trac 20814 restore the exact file where future import is not yet welcome

comment:21 Changed 5 years ago by
 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
 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
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
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
 Commit changed from d832810a08c0978cabaefd2bd86c310bb79ac993 to fc51d8e01998efca5901063886723ff0437f2bd7
Branch pushed to git repo; I updated commit sha1. New commits:
fc51d8e  tarc 20814 not pep8 for copyright header

comment:27 Changed 5 years ago by
 Branch changed from public/20814 to fc51d8e01998efca5901063886723ff0437f2bd7
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
more print converted to python3 in py files
more print converted to py3 in .py files