Opened 10 months ago

Closed 7 months ago

#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

Description

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 9 months ago.
Patch for #22840
22840-django-test.log (7.4 KB) - added by bowlofkashi 9 months ago.
django-tests log file

Download all attachments as: .zip

Change History (14)

comment:1 Changed 9 months ago by Deacalion

  • Cc matt@… added

comment:2 Changed 9 months ago by bowlofkashi

Hello,

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 9 months ago by bowlofkashi

  • Cc bowlofkashi added
  • Has patch set

comment:4 Changed 9 months ago by bowlofkashi

  • Has patch unset
  • Patch needs improvement set

comment:5 Changed 9 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 9 months ago by bowlofkashi

Patch for #22840

Changed 9 months ago by bowlofkashi

django-tests log file

comment:6 follow-up: Changed 9 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 9 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 8 months ago by Ricky

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

comment:9 Changed 8 months ago by bmispelon

comment:10 Changed 8 months ago by timo

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

comment:11 Changed 7 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 7 months ago by aaugustin (previous) (diff)

comment:12 Changed 7 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