Opened 4 years ago

Last modified 13 hours ago

#5812 enhancement new

PEP 3105 Make print a function

Reported by: thijs Owned by:
Priority: normal Milestone: Python-3.x
Component: core Keywords: review
Cc: thijs, teotwaki Branch: 5812-rodrigc-pep3105
(github, coverage, patch, buildbot, log)
Author: rodrigc

Description (last modified by rodrigc)

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 4 years ago.
0001-Converted-commented-print-statements-to-print-functi.patch (5.7 KB) - added by teotwaki 2 years ago.
0002-Converted-print-statement-to-print-function-in-test_.patch (720 bytes) - added by teotwaki 2 years ago.
0003-Added-print_function-import-from-__future__.patch (5.6 KB) - added by teotwaki 2 years ago.
print-tests-5812-rebase-conflict-fixes.patch (11.3 KB) - added by teotwaki 2 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 teotwaki 2 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 (36)

comment:1 Changed 4 years ago by thijs

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

comment:2 Changed 4 years ago by vperic

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 4 years ago by thijs

comment:3 Changed 4 years ago by thijs

  • Keywords review added

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

comment:4 follow-up: Changed 4 years ago by 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. :)

comment:5 in reply to: ↑ 4 Changed 4 years ago by thijs

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 4 years ago by thijs

  • Author set to thijs
  • Branch set to branches/print-tests-5812

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

comment:7 Changed 4 years ago by thijs

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

comment:8 Changed 4 years ago by thijs

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

comment:9 Changed 4 years ago by khorn

  • Owner set to khorn
  • Status changed from new to assigned

comment:10 Changed 4 years ago by khorn

  • Keywords review removed
  • Owner changed from khorn to thijs
  • Status changed from assigned to new

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

comment:11 Changed 2 years ago by teotwaki

  • Cc teotwaki added
  • Keywords review added

Hi thijs,

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

Changed 2 years ago by teotwaki

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

Changed 2 years ago by teotwaki

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 2 years ago by teotwaki

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

comment:13 Changed 2 years ago by adiroiban

  • Keywords review removed
  • Owner changed from thijs to teotwaki

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 5 days ago by rodrigc

  • Author changed from thijs to rodrigc
  • Branch branches/print-tests-5812 deleted
  • Description modified (diff)
  • Owner teotwaki deleted
  • Priority changed from low to normal
  • Summary changed from update print statement in twisted.test to PEP 3105 Make print a function | Python.org

comment:15 Changed 5 days ago by rodrigc

  • Summary changed from PEP 3105 Make print a function | Python.org to PEP 3105 Make print a function

comment:16 Changed 5 days ago by rodrigc

  • Branch set to 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 5 days ago by adiroiban

  • Keywords review removed
  • Owner set to rodrigc

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 5 days ago by rodrigc

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 4 days ago by adiroiban

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 4 days ago by adiroiban

  • Branch changed from https://github.com/twisted/twisted/pull/69 to 5812-rodrigc-pep3105

comment:21 Changed 4 days ago by rodrigc

  • Description modified (diff)

comment:22 Changed 4 days ago by rodrigc

  • Keywords review added
  • Owner changed from rodrigc to adiroiban
  • 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 3 days ago by adiroiban

  • Keywords review removed
  • Owner changed from adiroiban to rodrigc

Have you pushed your changes?

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

Is that ok?

comment:24 Changed 3 days ago by rodrigc

  • Keywords review added
  • Owner changed from rodrigc to adiroiban

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

Please review.

comment:25 Changed 3 days ago by adiroiban

I still see the only commit as 60c5495b8a82b9d4d0fa47ccdfb6e1b4e66bcb38

comment:26 Changed 3 days ago by adiroiban

ok so my initial review was for 14527b5bf8cc97d04c3cfd0b289dc5e0bdc9d5f6

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

comment:27 Changed 3 days ago by rodrigc

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 3 days ago by adiroiban

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 3 days ago by rodrigc

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 13 hours ago by adiroiban

  • Owner adiroiban deleted
Note: See TracTickets for help on using tickets.