Opened 5 years ago

Closed 12 months ago

Last modified 8 months ago

#5812 enhancement closed duplicate (duplicate)

PEP 3105 Make print a function

Reported by: Thijs Triemstra Owned by:
Priority: normal Milestone: Python-3.x
Component: core Keywords:
Cc: Thijs Triemstra, Sebastian Lauwers Branch: 5812-rodrigc-pep3105-2
branch-diff, diff-cov, branch-cov, buildbot
Author: rodrigc

Description (last modified by Craig Rodrigues)

PEP 3105 ( ​​​https://www.python.org/dev/peps/pep-3105/ ) print deprecates has changed from a statement to a function.

In Python 2.6 and Python 2.7, the default is print as a statement

print "Hello"

In Python 3.x, the above example is an error, since only print as a function is available.

To consistently use print as a function on Python 2.6, Python 2.7, and Python 3.x, this should be done:

from __future__ import print_function

print("Hello")

Attachments (6)

print-core-tests-5812.patch (12.2 KB) - added by Thijs Triemstra 5 years ago.
0001-Converted-commented-print-statements-to-print-functi.patch (5.7 KB) - added by Sebastian Lauwers 3 years ago.
0002-Converted-print-statement-to-print-function-in-test_.patch (720 bytes) - added by Sebastian Lauwers 3 years ago.
0003-Added-print_function-import-from-__future__.patch (5.6 KB) - added by Sebastian Lauwers 3 years ago.
print-tests-5812-rebase-conflict-fixes.patch (11.3 KB) - added by Sebastian Lauwers 3 years ago.
This patch is a rebase on top of TOT of the old print-test-5812 branch, fixing conflicts.
print-tests-5812-imports-and-comments.patch (18.0 KB) - added by Sebastian Lauwers 3 years ago.
This patch includes the previous patch, but also adds the correct future_statements for files that use print_function and converts commented print_statements

Download all attachments as: .zip

Change History (42)

comment:1 Changed 5 years ago by Thijs Triemstra

Should those prints that are commented out be fixed as well or removed?

comment:2 Changed 5 years ago by Vladimir Perić

I'd personally leave them as is (or delete them if they are obviously bogus).. on the other hand, there's like 10 of them so might as well convert them, no reason to stall this ticket because of that. BTW, you can use the 2to3 fixer to make these changes automatically: 2to3 -f print -w -n twisted/test and then just add the __future__ imports manually. At least all of these changes will be covered. :)

Changed 5 years ago by Thijs Triemstra

Attachment: print-core-tests-5812.patch added

comment:3 Changed 5 years ago by Thijs Triemstra

Keywords: review added

Attached patch replaces the active print() statements in twisted.test

comment:4 Changed 5 years ago by Vladimir Perić

In twisted.test.test_internet, there is a string, "resolve_helper", which then gets written to a file and executes, which has print statements. I guess it should be updated also.

Other than that, looks good to me, but last time I reviewed something I had to revert the change, so I'll let someone else take a look. :)

comment:5 in reply to:  4 Changed 5 years ago by Thijs Triemstra

Replying to vperic:

In twisted.test.test_internet, there is a string, "resolve_helper", which then gets written to a file and executes, which has print statements. I guess it should be updated also.

Other than that, looks good to me, but last time I reviewed something I had to revert the change, so I'll let someone else take a look. :)

I should've made a branch and run it against buildbot, let me do that here as well.

comment:6 Changed 5 years ago by Thijs Triemstra

Author: thijs
Branch: branches/print-tests-5812

(In [35052]) Branching to 'print-tests-5812'

comment:7 Changed 5 years ago by Thijs Triemstra

(In [35056]) apply print-core-tests-5812.patch, add news file. refs #5812

comment:8 Changed 5 years ago by Thijs Triemstra

The prints in resolve_helper were fixed and build results are here.

comment:9 Changed 5 years ago by khorn

Owner: set to khorn
Status: newassigned

comment:10 Changed 5 years ago by khorn

Keywords: review removed
Owner: changed from khorn to Thijs Triemstra
Status: assignednew

Looks pretty good, except:

The following files don't have:

from __future__ import print_function

and probably should for 2.6 compatibility.

twisted/test/iosim.py twisted/test/process_cmdline.py twisted/test/process_linger.py twisted/test/process_signal.py twisted/test/test_internet.py

Changed 3 years ago by Sebastian Lauwers

comment:11 Changed 3 years ago by Sebastian Lauwers

Cc: Sebastian Lauwers added
Keywords: review added

Hi thijs,

Attached are a few patches that provide the changes requested in the comments.

Changed 3 years ago by Sebastian Lauwers

This patch is a rebase on top of TOT of the old print-test-5812 branch, fixing conflicts.

Changed 3 years ago by Sebastian Lauwers

This patch includes the previous patch, but also adds the correct future_statements for files that use print_function and converts commented print_statements

comment:12 Changed 3 years ago by Sebastian Lauwers

To be clear, please ignore the three patches I posted yesterday (I don't know how to remove them).

comment:13 Changed 3 years ago by Adi Roiban

Keywords: review removed
Owner: changed from Thijs Triemstra to Sebastian Lauwers

Hi and thanks for the work in this!

I have reviewed the latest patch only. Looks good.

# print('X',self.x,'doing!')

and

# print(skey?) # print(e) # print(l[0].getBriefTraceback()) # print(repr(uj.dict)) # print("len", len(self.data)) # print("state2", self.state)

Do we need to keep these lines commented?

If needed, maybe add another comment to explain why it is needed.


Minor comment: In twisted/test/test_tcp.py have imports from future, but I don't see print method used in there.

---

Not sure if this indentation is compliant with code convention:

pyArgs = [pyExe, "-u", "-c", 
         "from __future__ import print_function; print('hello')"]

maybe

pyArgs = [
    pyExe, "-u", "-c", 
    "from __future__ import print_function; print('hello')",
    ] 

---

Please check the comments and let me know if you think that make sense!

Thanks!

comment:14 Changed 12 months ago by Craig Rodrigues

Author: thijsrodrigc
Branch: branches/print-tests-5812
Description: modified (diff)
Owner: Sebastian Lauwers deleted
Priority: lownormal
Summary: update print statement in twisted.testPEP 3105 Make print a function | Python.org

comment:15 Changed 12 months ago by Craig Rodrigues

Summary: PEP 3105 Make print a function | Python.orgPEP 3105 Make print a function

comment:16 Changed 12 months ago by Craig Rodrigues

Branch: https://github.com/twisted/twisted/pull/69
Keywords: review added

I have updated the patch submitted in this ticket, and submitted it as a GitHub pull request:

https://github.com/twisted/twisted/pull/69

Please review.

comment:17 Changed 12 months ago by Adi Roiban

Keywords: review removed
Owner: set to Craig Rodrigues

Thanks for the update.

Changes look good.

  1. I think that twisted/words/xish/xpathparser.py should be reverted as this is an auto-generated file.... you should fix it in xpathparser.g
  1. For twisted/words/im/pbsupport.py, twisted/web/sux.py I think that we should create a follow up ticket to replace the print calls to log calls.
  1. As commented in previous reviews, if we are looking at print statements, let's just clean unused code from twisted/conch/ssh/service.py twisted/conch/ui/tkvt100.py twisted/internet/_threadedselect.py twisted/mail/imap4.py twisted/names/authority.py twisted/names/client.py twisted/protocols/amp.py twisted/python/htmlizer.py twisted/test/crash_test_dummy.py twisted/test/test_banana.py twisted/test/test_dirdbm.py twisted/test/test_process.py twisted/web/domhelpers.py twisted/web/microdom.py
  1. There is also twisted/conch/test/test_manhole.py which has the code as string and used print statements ... same for twisted/scripts/test/test_scripts.py twisted/web/test/test_cgi.py
  1. There are still print statements in the docstrings from twisted/internet/defer.py and twisted/internet/inotify.py twisted/python/_textattributes.py twisted/python/components.py twisted/python/failure.py twisted/python/modules.py twisted/python/reflect.py
  1. there is still docs docs/conch/howto/conch_client.rst and more thinkgs in docs.

I am not sure what is the scope of this ticket.

Just a auto-conversion of print statement to print functions or a real code cleanup :)

The commented print statements can be followed up in a separate ticket.

But the ticket descriptions talks about consistent usage of the print function, so I think that this should also touch the docstrings and narrative docs

Please check my comments and resubmit.

Thanks!

comment:18 Changed 12 months ago by Craig Rodrigues

The scope of this ticket is print -> print() conversion only, not general code cleanup. The issues which you brought up are valid, but need to be in separate tickets, which I will file.

comment:19 Changed 12 months ago by Adi Roiban

Ok... so point 3 should be a separate ticket... but I think that the other comments should be addressed here as part of the "consistent usage of the print function" :)

Thanks!

comment:20 Changed 12 months ago by Adi Roiban

Branch: https://github.com/twisted/twisted/pull/695812-rodrigc-pep3105

comment:21 Changed 12 months ago by Craig Rodrigues

Description: modified (diff)

comment:22 Changed 12 months ago by Craig Rodrigues

Keywords: review added
Owner: changed from Craig Rodrigues to Adi Roiban
  • I removed xpathparser.g and xpathparser.py from this patch and created a new ticket 8350
  • I created a new ticket for cleanup removal of unused code 8349
  • I took a pass through the docs directories and fixed various rst and other files

Please review: https://github.com/twisted/twisted/pull/69

comment:23 Changed 12 months ago by Adi Roiban

Keywords: review removed
Owner: changed from Adi Roiban to Craig Rodrigues

Have you pushed your changes?

I can still see a single commit from 2 days ago.

Is that ok?

comment:24 Changed 12 months ago by Craig Rodrigues

Keywords: review added
Owner: changed from Craig Rodrigues to Adi Roiban

Yes, I pushed it, and the patch here is the latest: https://github.com/twisted/twisted/pull/69

Please review.

comment:25 Changed 12 months ago by Adi Roiban

I still see the only commit as 60c5495b8a82b9d4d0fa47ccdfb6e1b4e66bcb38

comment:26 Changed 12 months ago by Adi Roiban

ok so my initial review was for 14527b5bf8cc97d04c3cfd0b289dc5e0bdc9d5f6

it would help to see new commits on top of this commit

comment:27 Changed 12 months ago by Craig Rodrigues

I know this deviates from how Twisted has done things in the past, but it would be helpful if you could add your review comments in the GitHub pull request itself.

That's what hawkowl did in this pull request: https://github.com/twisted/twisted/pull/62 and it worked out.

comment:28 Changed 12 months ago by Adi Roiban

As a start, I don't want to change to many thinks in the same time.

I think that is important to have all feedback in the same place.

For now, the convention is to provide all feedback in these Trac comments.

I am not a big fan of this review, but I think that is better to be consistent.

In case all Twisted developers agree that all feedback for a ticket should be provided in the PR, I would be more than happy to do that...

but for now, my understanding is that the feedback for a PR should be provided in Trac.


Getting back to the feedback for this PR.

Where is the diff containing the changed made after the first review round?

This branch is quite big and would help to review only the changes.

Also, since the branch is so broad, it has a high change of producing merge conflicts... so we should review and merge it as soon as possible... or break it into smaller branches.

Thanks!

comment:29 Changed 12 months ago by Craig Rodrigues

Sorry, I don't have the original diff. I'll remember to do it that way next time around for a patch this large.

The stuff I changed in the updated diff is just all stuff in the docs directory which are not Python files. That would be:

  • docs/conch/howto/conch_client.rst
  • docs/core/examples/simple.tac
  • docs/core/howto/clients.rst
  • docs/core/howto/components.rst
  • docs/core/howto/cred.rst
  • docs/core/howto/defer-intro.rst
  • docs/core/howto/gendefer.rst
  • docs/core/howto/listings/pb/copy_receiver.tac
  • docs/core/howto/options.rst
  • docs/core/howto/pb-cred.rst
  • docs/core/howto/plugin.rst
  • docs/core/howto/process.rst
  • docs/core/howto/rdbms.rst
  • docs/core/howto/threading.rst
  • docs/core/howto/time.rst
  • docs/historic/2002/ipc10/twisted-network-framework/errata.html
  • docs/historic/2002/ipc10/twisted-network-framework/index.html
  • docs/historic/2003/europython/doanddont.html
  • docs/historic/2003/europython/tw-deploy.html
  • docs/historic/2003/europython/twisted.html
  • docs/historic/2003/europython/webclients.html
  • docs/historic/2003/pycon/applications/applications.html
  • docs/historic/2003/pycon/intrinsics-lightning/intrinsics-lightning
  • docs/historic/2003/pycon/releasing/releasing.html
  • docs/historic/2004/ibm/talk.html
  • docs/historic/Quotes/Twisted-1.0
  • docs/historic/ipc10errata.html
  • docs/historic/ipc10paper.html
  • docs/mail/examples/emailserver.tac
  • docs/mail/tutorial/smtpclient/smtpclient-10.tac
  • docs/mail/tutorial/smtpclient/smtpclient-11.tac
  • docs/mail/tutorial/smtpclient/smtpclient-7.tac
  • docs/mail/tutorial/smtpclient/smtpclient-8.tac
  • docs/mail/tutorial/smtpclient/smtpclient-9.tac
  • docs/mail/tutorial/smtpclient/smtpclient.rst
  • docs/names/howto/client-tour.rst
  • docs/web/howto/client.rst
  • docs/web/howto/web-in-60/session-endings.rst
  • docs/web/howto/web-in-60/session-store.rst
  • docs/web/howto/xmlrpc.rst

comment:30 Changed 12 months ago by Adi Roiban

Owner: Adi Roiban deleted

comment:31 Changed 12 months ago by Adi Roiban

Branch: 5812-rodrigc-pep31055812-rodrigc-pep3105-2

In the future please just merge your branch with trunk and solve the conflicts with mergetool, rather than doing a rebase.

With a rebase, you did solved the conflicts in your branch, but then conflicts will not be solved when merging your branch with the branch from the twisted repo :)

I have created a new branch

comment:32 Changed 12 months ago by Adi Roiban

Keywords: review removed
Owner: set to Craig Rodrigues

Changes looks good, but I think that we need better coverage.


I made a few updates... see my latest commit

  1. I saw that twisted/conch/test/test_manhole.py still used the print statement in testFunctionDefinition, testClassDefinition , test_controlE and test_controlA
  1. In the docstring from twisted/protocols/amp.py there were still places in which the examples were using the print statement
  1. In twisted/docs/core/examples/simple.tac docs/core/howto/options.rst there was still an instance of print statement
  1. Also in docs/conch/howto/conch_client.rst

See the coverage report for your commit https://codecov.io/gh/twisted/twisted/commit/ab4a32a

It shows that some lines are not executed at runtime.

importing code with print 1 will raise a syntax error on python 3, importing a code with print(1) will not raise any error at compile time

to make sure the changes work, we need to actual run the modified code.


Please update your branch to include test code exercising all the changed lines.

You might want to break this into multiple branches to make it easier to merge the changes.

For example you can put all documentation and docstring changes in a separate branch and they will land with only a human review, as for now we don't have tests for the documentation code.

You can also break into a branch containing print function changes which are already covered by the existing tests.


If you know there is other way to check that the current changes don't break the python2.7 code, I am happy to investigate.


If you think that this branch should land as it is, feel free to put it again for review. From my conversation over the mailing list, my understanding was that all porting changes need at least 100% coverage.

Many thanks for your contribution.

comment:33 Changed 12 months ago by Craig Rodrigues

Keywords: review added
Owner: changed from Craig Rodrigues to Adi Roiban

Thanks for doing the analysis and adding additional print() conversions.

codecov.io looks like an interesting took, but I don't know how to interpret the results.

Can you redo your review and list the files that have sufficient code coverage in the patch? What would be fine with me is if you took just those files with sufficient coverage and committed them right now, and listed the files that cannot be committed because more tests need to be written.

This would allow some forward progress, and help reduce the size of future patches.

comment:34 Changed 12 months ago by Adi Roiban

Owner: Adi Roiban deleted

OK. NP.

I don't know if I would have time in the near future to redo the review.

Leaving this for review, in case someone else have time to break this branch.

comment:35 Changed 12 months ago by Craig Rodrigues

Resolution: duplicate
Status: newclosed

This patch is a bit too big to handle in a review in one ticket, so I split up the patch into separate tickets, to make things a bit easier to handle:

comment:36 Changed 8 months ago by hawkowl

Keywords: review removed

[mass edit] Removing review from closed tickets.

Note: See TracTickets for help on using tickets.