Opened 4 years ago

Closed 4 weeks ago

#5812 enhancement closed duplicate (duplicate)

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-2
branch-diff, diff-cov, branch-cov, buildbot
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 (41)

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 weeks 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 weeks 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 weeks 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 weeks 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 weeks 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 5 weeks 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 5 weeks ago by adiroiban

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

comment:21 Changed 5 weeks ago by rodrigc

  • Description modified (diff)

comment:22 Changed 5 weeks 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 5 weeks 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 5 weeks 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 5 weeks ago by adiroiban

I still see the only commit as 60c5495b8a82b9d4d0fa47ccdfb6e1b4e66bcb38

comment:26 Changed 5 weeks 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 5 weeks 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 5 weeks 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 5 weeks 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 5 weeks ago by adiroiban

  • Owner adiroiban deleted

comment:31 Changed 4 weeks ago by adiroiban

  • Branch changed from 5812-rodrigc-pep3105 to 5812-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 4 weeks ago by adiroiban

  • Keywords review removed
  • Owner set to rodrigc

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 4 weeks ago by rodrigc

  • Keywords review added
  • Owner changed from rodrigc to adiroiban

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

  • Owner adiroiban 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 4 weeks ago by rodrigc

  • Resolution set to duplicate
  • Status changed from new to closed

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:

Note: See TracTickets for help on using tickets.