Code

Opened 7 years ago

Closed 7 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@…> 7 years ago.
Replaced iter attribute lookup by trial an error with iter()
iterables.2.patch (3.4 KB) - added by leosoto <leo.soto@…> 7 years ago.
added a missing header on the diff file
iterables2.patch (3.5 KB) - added by leosoto <leo.soto@…> 7 years ago.
another patch following malcom's suggestions
iterables3.patch (4.6 KB) - added by leosoto <leo.soto@…> 7 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@…> 7 years ago.
The previous patch file is messed up. Now it's OK.

Download all attachments as: .zip

Change History (11)

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

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

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

added a missing header on the diff file

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

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Ready for checkin

comment:2 Changed 7 years ago by mtredinnick

  • Triage Stage changed from Ready for checkin to Accepted

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 7 years ago by leosoto <leo.soto@…>

another patch following malcom's suggestions

comment:3 Changed 7 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 7 years ago by Fredrik Lundh <fredrik@…>

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

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

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 7 years ago by leosoto <leo.soto@…>

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

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

  • Keywords jython, sprintsept14 added; jython removed
  • Triage Stage changed from Accepted to Ready 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 7 years ago by jacob

  • Resolution set to fixed
  • Status changed from new to closed

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.