[Django] #25225: Unnecessary/Redundant __iter__ method with ListMixin class (contrib.gis.geos.mutable_list.py)?
Django
(contrib.gis.geos.mutable_list.py)?
-------------------------------------+-------------------------------------
Reporter: bixb0012 | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: GIS | Version: 1.8
Severity: Normal | Keywords: __iter__ geos gis
| ListMixin
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
The ListMixin class (contrib.gis.geos.mutable_list.py) currently has an
__iter__ method and __getitem__ method. Typically iteration is
implemented using one or the other, not both. Looking at the two methods,
the __iter__ method appears to be unnecessary since the Python interpreter
will use __getitem__ to iterate if __iter__ is not present. Also,
stepping through code appears to have __iter__ call __getitem__, which
raises the question of why define an __iter__ method when the interpreter
can use __getitem__ directly.
Currently, __getitem__ can't be used properly for iteration because of the
custom IndexError raised in the _checkindex method. Switching from
raising a django.contrib.gis.geos.error.GEOSIndexError exception to a
exceptions.IndexError will allow for the interpreter to use __getitem__
for iteration, which would allow removing the unnecessary or redundant
__iter__ method.
--
Ticket URL: <https://code.djangoproject.com/ticket/25225>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
(contrib.gis.geos.mutable_list.py)?
-------------------------------------+-------------------------------------
Reporter: bixb0012 | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: GIS | Version: 1.8
Keywords: __iter__ geos gis | Triage Stage:
ListMixin | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_docs: => 0
* needs_tests: => 0
* needs_better_patch: => 0
Old description:
> The ListMixin class (contrib.gis.geos.mutable_list.py) currently has an
> __iter__ method and __getitem__ method. Typically iteration is
> implemented using one or the other, not both. Looking at the two
> methods, the __iter__ method appears to be unnecessary since the Python
> interpreter will use __getitem__ to iterate if __iter__ is not present.
> Also, stepping through code appears to have __iter__ call __getitem__,
> which raises the question of why define an __iter__ method when the
> interpreter can use __getitem__ directly.
>
> Currently, __getitem__ can't be used properly for iteration because of
> the custom IndexError raised in the _checkindex method. Switching from
> raising a django.contrib.gis.geos.error.GEOSIndexError exception to a
> exceptions.IndexError will allow for the interpreter to use __getitem__
> for iteration, which would allow removing the unnecessary or redundant
> __iter__ method.
New description:
The `ListMixin` class (`contrib.gis.geos.mutable_list.py`) currently has
an `__iter__` method and `__getitem__` method. Typically iteration is
implemented using one or the other, not both. Looking at the two methods,
the `__iter__`method appears to be unnecessary since the Python
interpreter will use `__getitem__`to iterate if `__iter__` is not present.
Also, stepping through code appears to have `__iter__` call `__getitem__`,
which raises the question of why define an `__iter__` method when the
interpreter can use `__getitem__`directly.
Currently, `__getitem__` can't be used properly for iteration because of
the custom `IndexError` raised in the `_checkindex` method. Switching
from raising a `django.contrib.gis.geos.error.GEOSIndexError` exception to
a `exceptions.IndexError` will allow for the interpreter to use
`__getitem__`for iteration, which would allow removing the unnecessary or
redundant `__iter__` method.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/25225#comment:1>
Django
(contrib.gis.geos.mutable_list.py)?
-------------------------------------+-------------------------------------
Reporter: bixb0012 | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: GIS | Version: 1.8
Keywords: __iter__ geos gis | Triage Stage:
ListMixin | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by timgraham):
* easy: 1 => 0
Comment:
Could you propose a patch with a regression test?
--
Ticket URL: <https://code.djangoproject.com/ticket/25225#comment:2>
Django
(contrib.gis.geos.mutable_list.py)?
-------------------------------------+-------------------------------------
Reporter: bixb0012 | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: GIS | Version: 1.8
Keywords: __iter__ geos gis | Triage Stage:
ListMixin | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timgraham):
It looks like #4740 is the reason the way this code is the way it is, but
the template engine lookups now catch `IndexError` too, so we may be able
to remove the custom exception.
--
Ticket URL: <https://code.djangoproject.com/ticket/25225#comment:3>
Django
(contrib.gis.geos.mutable_list.py)?
-------------------------------------+-------------------------------------
Reporter: bixb0012 | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: GIS | Version: 1.8
Keywords: __iter__ geos gis | Triage Stage: Accepted
ListMixin |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
* stage: Unreviewed => Accepted
Comment:
[https://github.com/django/django/pull/5132 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/25225#comment:4>
Django
(contrib.gis.geos.mutable_list.py)?
-------------------------------------+-------------------------------------
Reporter: bixb0012 | Owner: nobody
Cleanup/optimization |
Component: GIS | Version: 1.8
Keywords: __iter__ geos gis | Triage Stage: Accepted
ListMixin |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"197b1878105504b5ac7e399e1f52a6093c88baa3" 197b187]:
{{{
#!CommitTicketReference repository=""
revision="197b1878105504b5ac7e399e1f52a6093c88baa3"
Fixed #25225 -- Simplified code to remove GEOSIndexError
The test is a regression for refs #4740 to show that the original
fix of GEOSIndexError is no longer needed.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25225#comment:5>
Django
(contrib.gis.geos.mutable_list.py)?
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: GIS | Version: 1.8
Keywords: __iter__ geos gis | Triage Stage: Accepted
ListMixin |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"35800acf79acf637f9afab17f60ad7ec4aacb9ce" 35800ac]:
{{{
#!CommitTicketReference repository=""
revision="35800acf79acf637f9afab17f60ad7ec4aacb9ce"
Refs #25225 -- Removed test for removed ListMixin._IndexError.
Unused since 197b1878105504b5ac7e399e1f52a6093c88baa3.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25225#comment:6>