Opened 10 years ago

Closed 10 years ago

#22840 closed Cleanup/optimization (wontfix)

Use six.integer_types everywhere

Reported by: Marc Tamlyn Owned by: anonymous
Component: Python 2 Version: dev
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 10 years ago.
Patch for #22840
22840-django-test.log (7.4 KB ) - added by bowlofkashi 10 years ago.
django-tests log file

Download all attachments as: .zip

Change History (14)

comment:1 by Matt Deacalion Stevens, 10 years ago

Cc: matt@… added

comment:2 by bowlofkashi, 10 years ago

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 by bowlofkashi, 10 years ago

Cc: bowlofkashi added
Has patch: set

comment:4 by bowlofkashi, 10 years ago

Has patch: unset
Patch needs improvement: set

comment:5 by bowlofkashi, 10 years ago

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.

by bowlofkashi, 10 years ago

Patch for #22840

by bowlofkashi, 10 years ago

Attachment: 22840-django-test.log added

django-tests log file

comment:6 by Tim Graham, 10 years ago

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.

in reply to:  6 comment:7 by bowlofkashi, 10 years ago

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 by Ricky, 10 years ago

Owner: changed from nobody to anonymous
Status: newassigned

comment:9 by Baptiste Mispelon, 10 years ago

comment:10 by Tim Graham, 10 years ago

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

comment:11 by Aymeric Augustin, 10 years ago

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 10 years ago by Aymeric Augustin (previous) (diff)

comment:12 by Tim Graham, 10 years ago

Resolution: wontfix
Status: assignedclosed
Note: See TracTickets for help on using tickets.
Back to Top