Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#5689 task closed fixed (fixed)

Remove uses of classical division

Reported by: Vladimir Perić Owned by: Vladimir Perić
Priority: normal Milestone: Python-3.x
Component: core Keywords: py3k
Cc: Branch: branches/classical-division-5689
branch-diff, diff-cov, branch-cov, buildbot
Author: vperic

Description (last modified by therve)

The "classical" division operator "/" has been deprecated for a long while (Python 2.2 I believe) as it is ambiguous - it means floor division with ints, but true division otherwise. In Python 3, the "/" operator is for true division (always returns a float), while the unambiguous operator "" should be used for floor division. As such, it is required to explicitly either import division from __future__ or use the floor division operator.

Most of the time, division was used to force a ZeroDivisionError, for testing purposes. In such cases I've usually imported from __future__, though in some cases I just changed the line to use the floor operator (as the effect is the same). The only places where floor division actually needed to be used are:

  • twisted/conch/connection.py
  • twisted/mail/imap4.py
  • twisted/mail/smtp.py (date conversion for these two)
  • twisted/test/test_protocols.py (can't use floats to call range())
  • twisted/web/test/test_webclient.py (ditto)
  • twisted/web/test/test_xml.py
  • twisted/web/twcgi.py
  • twisted/words/test/test_irc.py

Attachments (1)

classic-division.patch (18.7 KB) - added by Vladimir Perić 5 years ago.

Download all attachments as: .zip

Change History (10)

Changed 5 years ago by Vladimir Perić

Attachment: classic-division.patch added

comment:1 Changed 5 years ago by therve

Description: modified (diff)

comment:2 Changed 5 years ago by Itamar Turner-Trauring

Keywords: review removed
Owner: set to Vladimir Perić

I'm not sure what the point of switching to a less ambiguous version of '1/0' is, since it will do ZeroDivisionError regardless. But I guess the coding standard is going to require non-ambiguity, so I suppose we should do this.

So, looks good - do the usual buildbot run, then merge if it passes (and don't forget to add this to the coding standard ticket).

Also - you might want to mention this change on the mailing list, so people are on the lookout for from __future__ import division. You might want to mention *all* Python 3 coding standard changes, but this one in particular impacts the behavior of whole modules. An argument could be made that we should *require* it on all twisted modules, much like we want all new classes to be new style, so you should bring that up.

comment:3 Changed 5 years ago by Vladimir Perić

Branch: branches/classical-division-5689

(In [34519]) Create branch classical-division-5689

comment:4 Changed 5 years ago by Vladimir Perić

comment:5 Changed 5 years ago by Vladimir Perić

BTW, the point of switching to the less ambiguous version is to get rid of the warnings thrown by "-3", so we can notice the actual errors: out of about 120 errors around 20 were real, and this would've been quite hard to notice without fixing everything else. Also, the whole test suite spews out _a lot_ of warnings, making it unwieldy to work with.

I'll write to the list today or tomorrow, and submit the coding standards ticket afterwards (perhaps people will disagree with my suggestions).

Anyway, builds seem to be doing good, so I'll go ahead and merge once the last few Windows bots finish.

comment:6 Changed 5 years ago by Vladimir Perić

Resolution: fixed
Status: newclosed

Well, I guess trac didn't catch it correctly, but this is now merged. Closing the ticket manually.

comment:7 Changed 5 years ago by Jean-Paul Calderone

It's very unfortunate that trac has this problem. The correct way to respond to it is covered on CommitterCheckList though. Please re-read that page to refresh your memory of this and other commit-related tasks you might want to accomplish. Thanks.

comment:8 Changed 5 years ago by Vladimir Perić

(In [34523]) Merge classical-division-5689: Remove uses of classical division

Author: vperic Reviewer: itamar Fixes: #5689

The "classical" division operator "/" has been deprecated for a long while (Python 2.2 I believe) as it is ambiguous - it means floor division with ints, but true division otherwise. In Python 3, the "/" operator is for true division (always returns a float), while the unambiguous operator "" should be used for floor division. As such, it is required to explicitly either import division from future or use the floor division operator.

Most of the time, division was used to force a ZeroDivisionError, for testing purposes. In such cases, I've usually imported from future, though in some cases I just changed the line to use the floor operator. These changes are still important, as they will hide the warnings produced by "-3" and make it easier to spot the real errors (of which there were about twenty).

comment:9 Changed 5 years ago by Vladimir Perić

Hi exarkun, thanks for fixing this. I did follow the steps, I checked the post-commit log but couldn't find any errors reported, so I brought it up on IRC and itamar told me to just close it manually. I guess I just missed the relevant line, I'll be more careful if it happens again.

Note: See TracTickets for help on using tickets.