Opened 6 months ago

Closed 6 months ago

#34983 closed Cleanup/optimization (fixed)

Deprecate django.utils.itercompat.is_iterable().

Reported by: Nick Pope Owned by: Nick Pope
Component: Utilities Version: dev
Severity: Normal Keywords: itercompat, is_iterable
Cc: Carsten Fuchs Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

While reviewing the PR for #10941, I saw that we are still using the django.utils.itercompat.is_iterable() helper function.

It seems that this was originally added in ad077ccbc03f11c3db3951aab110ec98c5907729 to fix #5445 back in 2007. This was required because Jython 2.2 didn't have support for __iter__(). Django's support for Jython was removed in 2017 in #28954.

As pointed out in this comment, is_iterable() also handles the older fallback to determining if something is iterable by the presence of __getitem__() as mentioned in Python's documentation:

Checking isinstance(obj, Iterable) detects classes that are registered as Iterable or that have an __iter__() method, but it does not detect classes that iterate with the __getitem__() method. The only reliable way to determine whether an object is iterable is to call iter(obj).

In practice, however, it is rare that iterables do not support __iter__() and, for all uses of this internal function, it doesn't seem to be necessary.

I propose that we replace this function with the more standard isinstance(..., collections.abc.Iterable) and deprecate it. (Although the function is private and undocumented, it is likely used in the wild and other past functions from this module have been through a deprecation process.)

Change History (12)

comment:1 by Nick Pope, 6 months ago

Has patch: set

comment:2 by Mariusz Felisiak, 6 months ago

Thanks for the ticket. Have you checked how it looks like for PyPy? I'm afraid removing it will make things worse for PyPy. For example ChoiceIterators don't use is_iterable() and they seem to be broken on PyPy:

./runtests.py model_fields
....
django.core.management.base.SystemCheckError: SystemCheckError: System check identified some issues:

ERRORS:
model_fields.Choiceful.choices_from_callable: (fields.E004) 'choices' must be a mapping (e.g. a dictionary) or an iterable (e.g. a list or tuple).
model_fields.Choiceful.with_choices_dict: (fields.E005) 'choices' must be a mapping of actual values to human readable names or an iterable containing (actual value, human readable name) tuples.
model_fields.Choiceful.with_choices_nested_dict: (fields.E005) 'choices' must be a mapping of actual values to human readable names or an iterable containing (actual value, human readable name) tuples.
model_fields.Whiz.c: (fields.E005) 'choices' must be a mapping of actual values to human readable names or an iterable containing (actual value, human readable name) tuples.
model_fields.WhizDelayed.c: (fields.E005) 'choices' must be a mapping of actual values to human readable names or an iterable containing (actual value, human readable name) tuples.
model_fields.WhizIter.c: (fields.E005) 'choices' must be a mapping of actual values to human readable names or an iterable containing (actual value, human readable name) tuples.

System check identified 6 issues (0 silenced).

comment:3 by Nick Pope, 6 months ago

I'll dig into the issues around the choices stuff some more, but in the general simple case it seems like it should be fine:

❯ pypy3
Python 3.10.13 (f1607341da97ff5a1e93430b6e8c4af0ad1aa019, Oct 02 2023, 19:25:21)
[PyPy 7.3.13 with GCC 13.2.1 20230801] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>>> from collections.abc import Iterable
>>>> class A:
....     def __iter__(self):
....         pass
....         
>>>> isinstance(A(), Iterable)
True
>>>> 

comment:4 by Mariusz Felisiak, 6 months ago

It seems that 500e01073adda32d5149624ee9a5cb7aa3d3583f introduces a big regression on PyPy, e.g.

  • before we had 3 failures in forms_tests, after failures=13, errors=32,
  • before we had no failures in model_fields, after system check identifies issues, etc.

comment:5 by Carsten Fuchs, 6 months ago

Cc: Carsten Fuchs added

in reply to:  4 comment:6 by Natalia Bidart, 6 months ago

Replying to Mariusz Felisiak:

It seems that 500e01073adda32d5149624ee9a5cb7aa3d3583f introduces a big regression on PyPy, e.g.

While doing the review for that PR, I did check PyPy compatibility. Is that something we should be doing? Thanks!

comment:7 by Nick Pope, 6 months ago

So 3.10 support in PyPy is still quite new - less than 6 months old.

It looks like we've found a bug for which I've opened an issue: https://foss.heptapod.net/pypy/pypy/-/issues/4036

That is not related to this ticket, however - what we're looking at here seems to work fine.

comment:8 by Mariusz Felisiak, 6 months ago

Triage Stage: UnreviewedAccepted

Tentatively accepted.

comment:9 by Mariusz Felisiak, 6 months ago

Patch needs improvement: set

comment:10 by Nick Pope, 6 months ago

Patch needs improvement: unset

comment:11 by Mariusz Felisiak, 6 months ago

Triage Stage: AcceptedReady for checkin

comment:12 by GitHub <noreply@…>, 6 months ago

Resolution: fixed
Status: assignedclosed

In 5e28cd3f:

Fixed #34983 -- Deprecated django.utils.itercompat.is_iterable().

Note: See TracTickets for help on using tickets.
Back to Top