[Django] #22840: Use six.integer_types everywhere
Django
------------------------------------------------+------------------------
Reporter: mjtamlyn | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Python 2 | Version: master
Severity: Normal | Keywords:
Triage Stage: Accepted | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
------------------------------------------------+------------------------
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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/22840>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
--------------------------------------+------------------------------------
Reporter: mjtamlyn | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Python 2 | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Deacalion):
* cc: matt@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/22840#comment:1>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Python 2 | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (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!)
--
Ticket URL: <https://code.djangoproject.com/ticket/22840#comment:2>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Python 2 | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by bowlofkashi):
* cc: bowlofkashi (added)
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/22840#comment:3>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Python 2 | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by bowlofkashi):
* needs_better_patch: 0 => 1
* has_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/22840#comment:4>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Python 2 | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22840#comment:5>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Python 2 | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (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
[https://docs.djangoproject.com/en/dev/internals/contributing/writing-code
/submitting-patches/#patch-review-checklist patch review checklist]. By
the way, there's no need to attach a log of the test output.
--
Ticket URL: <https://code.djangoproject.com/ticket/22840#comment:6>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Python 2 | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by bowlofkashi):
Replying to [comment:6 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22840#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: mjtamlyn | Owner: anonymous
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: Python 2 | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Changes (by Ricky):
* status: new => assigned
* owner: nobody => anonymous
--
Ticket URL: <https://code.djangoproject.com/ticket/22840#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: mjtamlyn | Owner: anonymous
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: Python 2 | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by bmispelon):
Work has started on a branch here:
https://github.com/riccardotonini/django/tree/ticket-22840
--
Ticket URL: <https://code.djangoproject.com/ticket/22840#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: mjtamlyn | Owner: anonymous
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: Python 2 | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by timo):
Is there any rebuttal to my comment 6 where I questioned the usefulness of
this?
--
Ticket URL: <https://code.djangoproject.com/ticket/22840#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: mjtamlyn | Owner: anonymous
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: Python 2 | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by aaugustin):
Like timo, I'm a 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22840#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: mjtamlyn | Owner: anonymous
Cleanup/optimization | Version: master
Component: Python 2 | Resolution: wontfix
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => wontfix
--
Ticket URL: <https://code.djangoproject.com/ticket/22840#comment:12>