Opened 9 years ago

Closed 9 years ago

#5445 closed (fixed)

Checking iterables by looking for a __iter__ attribute doesn't work on Jython

Reported by: leosoto <leo.soto@…> Owned by: nobody
Component: Core (Other) Version: master
Severity: Keywords: jython, sprintsept14
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Jython list and tuples are iterables but don't have iter, so are incorrectly detected as non-iterables by model validation.

Attachments (5)

iterables.patch (3.4 KB) - added by leosoto <leo.soto@…> 9 years ago.
Replaced iter attribute lookup by trial an error with iter()
iterables.2.patch (3.4 KB) - added by leosoto <leo.soto@…> 9 years ago.
added a missing header on the diff file
iterables2.patch (3.5 KB) - added by leosoto <leo.soto@…> 9 years ago.
another patch following malcom's suggestions
iterables3.patch (4.6 KB) - added by leosoto <leo.soto@…> 9 years ago.
Uses is_iterable on django.template too. Also changes the order of the typical check for the common case: now we first check for non-strings, next for iterables.
iterables3.2.patch (4.5 KB) - added by leosoto <leo.soto@…> 9 years ago.
The previous patch file is messed up. Now it's OK.

Download all attachments as: .zip

Change History (11)

Changed 9 years ago by leosoto <leo.soto@…>

Attachment: iterables.patch added

Replaced iter attribute lookup by trial an error with iter()

Changed 9 years ago by leosoto <leo.soto@…>

Attachment: iterables.2.patch added

added a missing header on the diff file

comment:1 Changed 9 years ago by Simon G. <dev@…>

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedReady for checkin

comment:2 Changed 9 years ago by Malcolm Tredinnick

Triage Stage: Ready for checkinAccepted

I'm not in love with the implementation. A couple of comments:

  • you create _is_iterable in validation.py and then later on do almost the exact same thing in test/client.py. Why not create a file in django/utils/ called something appropriate (or use itercompat.py, which we already have) and put a utility function in there?
  • Does Jython have a basestring type? We use isinstance(foo, basestring) all over the place in Django, so it'd be nice we could also use that here instead of types.StringType. Otherwise there are a lot of places you're going to have to change.

Changed 9 years ago by leosoto <leo.soto@…>

Attachment: iterables2.patch added

another patch following malcom's suggestions

comment:3 Changed 9 years ago by Fredrik Lundh <fredrik@…>

Looks reasonable. Not sure about the name "itercompat", though. CPython does not guarantee that all iterables have an iter method; they may just implement the tp_iter slot on the C level, or just provide a C-level sequence API. iter() handles all those cases.

comment:4 Changed 9 years ago by Fredrik Lundh <fredrik@…>

(oh, sorry, missed that itercompat.py is an existing module. +1 from me, then)

Changed 9 years ago by leosoto <leo.soto@…>

Attachment: iterables3.patch added

Uses is_iterable on django.template too. Also changes the order of the typical check for the common case: now we first check for non-strings, next for iterables.

Changed 9 years ago by leosoto <leo.soto@…>

Attachment: iterables3.2.patch added

The previous patch file is messed up. Now it's OK.

comment:5 Changed 9 years ago by Fredrik Lundh <fredrik@…>

Keywords: sprintsept14 added
Triage Stage: AcceptedReady for checkin

Looks good to me. If you've missed something else, we can deal with that in an additional issue. Boldly tagging this and marking it as "Read for checkin".

comment:6 Changed 9 years ago by Jacob

Resolution: fixed
Status: newclosed

(In [6211]) Fixed #5445: added some compatibility code for the lack of iter in Jython 2.2. Thanks, Leo Soto.

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