#22840 closed Cleanup/optimization (wontfix)

Use six.integer_types everywhere

Reported by: mjtamlyn Owned by: anonymous
Component: Python 2 Version: master
Severity: Normal Keywords:
Cc: matt@…, bowlofkashi Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: yes UI/UX: no


There are 23 places in the source code where we use isinstance(..., int) rather than six.integer_types. There is probably no reason why these should not accept instances of long on python 2, so we should update these instances (with regression tests) to be six.integer_types.

Attachments (2)

22840-django-patch-six-integer-types.patch (12.8 KB) - added by bowlofkashi 17 months ago.
Patch for #22840
22840-django-test.log (7.4 KB) - added by bowlofkashi 17 months ago.
django-tests log file

Download all attachments as: .zip

Change History (14)

comment:1 Changed 18 months ago by Deacalion

  • Cc matt@… added

comment:2 Changed 17 months ago by bowlofkashi


This will be my first contribution to Django! I have found the occurrences of isinstance(..., int) and have changed them to isinstance(..., six.integer_types). I ran tests on Django using the command PYTHONPATH=..:$PYTHONPATH ./runtests.py

(I could not figure out how to make the bolded text monospaced, so I made it bold. Sorry!)

comment:3 Changed 17 months ago by bowlofkashi

  • Cc bowlofkashi added
  • Has patch set

comment:4 Changed 17 months ago by bowlofkashi

  • Has patch unset
  • Patch needs improvement set

comment:5 Changed 17 months ago by bowlofkashi

Sorry. That was a bad patch. I am making improvements. I could not find how to remove the bad patch file attachment. Please don't use it.

Changed 17 months ago by bowlofkashi

Patch for #22840

Changed 17 months ago by bowlofkashi

django-tests log file

comment:6 follow-up: Changed 17 months ago by timo

Thanks for your work so far. As noted in the description, we need regression tests for these modifications. That is for each change, there should be a test that fails before the change and passes afterward. That said, I am not convinced we should make these changes everywhere especially in places like logging where I don't think the logging level would every be a long(). Further, we'd have to skip the tests on Python 3 since long() doesn't exist there. Once we drop Python 2, we'd then just revert this change since it's no longer needed. All in all, it seems like a lot of perhaps unnecessary work to blindly make this change without some justification for each case (e.g. #22820).

As a new contributor, you may interested in our patch review checklist. By the way, there's no need to attach a log of the test output.

comment:7 in reply to: ↑ 6 Changed 17 months ago by bowlofkashi

Replying to timo:
Thank you for your response and your help. I will read the patch review checklist, as well as the guidelines set for the project.

comment:8 Changed 16 months ago by Ricky

  • Owner changed from nobody to anonymous
  • Status changed from new to assigned

comment:9 Changed 16 months ago by bmispelon

comment:10 Changed 16 months ago by timo

Is there any rebuttal to my comment 6 where I questioned the usefulness of this?

comment:11 Changed 15 months ago by aaugustin

Like timo, I'm skeptical about this change.

If someone wants to audit each case and see if longs could cause an issue, why not.

But if it was a common cause for bugs we would have heard about it before.

Last edited 15 months ago by aaugustin (previous) (diff)

comment:12 Changed 15 months ago by timgraham

  • Resolution set to wontfix
  • Status changed from assigned to closed
Note: See TracTickets for help on using tickets.
Back to Top