Opened 8 years ago

Closed 8 years ago

#3184 closed defect (fixed)

[patch] better unordered_list

Reported by: dummy@… Owned by: adrian
Component: Template system Version: master
Severity: normal Keywords:
Cc: simonbun@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

If you have a var = ['item 1', 'item 2', 'item 3'] and do {{ var|unordered_list }} you will get an IndexError:

IndexError at ...
string index out of range
Request Method: GET
Request URL: ...
Exception Type: IndexError
Exception Value: string index out of range
Exception Location: /usr/local/lib/python2.4/site-packages/django/template/defaultfilters.py in _helper, line 292

I would expect a simple result like this:

<li>item 1</li>
<li>item 2</li>
<li>item 3</li>

I made some patch for unordered_list and the regressiontest.

Attachments (10)

defaultfilters.diff (1.1 KB) - added by dummy@… 8 years ago.
better unordered_list
defaultfilters-test.diff (718 bytes) - added by dummy@… 8 years ago.
diff for regressiontest
defaultfilters-test.2.diff (1.2 KB) - added by dummy@… 8 years ago.
new version, more common use case
defaultfilters-test.3.diff (1.2 KB) - added by dummy@… 8 years ago.
new version, more common use case
defaultfilters.2.diff (1.9 KB) - added by dummy@… 8 years ago.
new version, more common use case
defaultfilters.3.diff (1.6 KB) - added by dummy@… 8 years ago.
hopefully my last version of the patch
defaultfilters.4.diff (1.6 KB) - added by dummy@… 8 years ago.
hopefully my last version of the patch
defaultfilters-test.4.diff (1.7 KB) - added by dummy@… 8 years ago.
hopefully my last version of the patch
better-unordered.diff (4.1 KB) - added by Dirk Datzert <dummy@…> 8 years ago.
I made 1 whole patch, and added the failing testcase
better-unordered.patch (6.1 KB) - added by SmileyChris 8 years ago.
completely rewritten with tests and docs

Download all attachments as: .zip

Change History (20)

Changed 8 years ago by dummy@…

better unordered_list

Changed 8 years ago by dummy@…

diff for regressiontest

Changed 8 years ago by dummy@…

new version, more common use case

Changed 8 years ago by dummy@…

new version, more common use case

Changed 8 years ago by dummy@…

new version, more common use case

Changed 8 years ago by dummy@…

hopefully my last version of the patch

Changed 8 years ago by dummy@…

hopefully my last version of the patch

Changed 8 years ago by dummy@…

hopefully my last version of the patch

comment:1 Changed 8 years ago by SmileyChris

Good job on the patch! I always found using unordered_list seemed more difficult than it needed to be.

comment:2 Changed 8 years ago by simonbun <simonbun@…>

  • Cc simonbun@… added

Hi, thanks for the patch.

I've mailed you about a small bug in the patch with arrays of 2+ depth. Test case:

['page1', ['page2', ['page3', ['page4']]]]

At first i tried patching your patch, but i ended up rewriting it because it was too complex for me.

I decided not to post my code as a diff seeing as it's not quite finished. The original filter inserted newlines and tabs so that the html was formatted nicely. Personally i have no need for this feature, so i didn't implement it.

It shouldn't be much work though, so if someone is still reading this, feel free to implement it.

code:

def unordered_list(value):
    
  def _recurse_children(parent):
    temp = ''
    
    for child in parent:
      temp += '<li>'
      
      if type(child) == ListType:
        temp += '<ul>' + _recurse_children(child) + '</ul>'
      else:
        temp += child
        
      temp += '</li>'
      
    return temp
      
  return _recurse_children(value)

Regards,
Simon

comment:3 follow-up: Changed 8 years ago by simonbun <simonbun@…>

Hold on, child <ul>'s were not nested correctly. This is better:

def unordered_list(value):
    
  def _recurse_children(parent):
    temp = ''
    
    for child in parent:
      if type(child) == ListType:
        temp += '<ul>' + _recurse_children(child) + '</ul>'
      else:
        if temp != '':
          temp += '</li>'
        
        temp += '<li>' + child
      
    return temp
      
  return _recurse_children(value)

comment:4 in reply to: ↑ 3 Changed 8 years ago by dummy@…

Replying to simonbun <simonbun@versea.be>:

Hold on, child <ul>'s were not nested correctly. This is better:

def unordered_list(value):
    
  def _recurse_children(parent):
    temp = ''
    
    for child in parent:
      if type(child) == ListType:
        temp += '<ul>' + _recurse_children(child) + '</ul>'
      else:
        if temp != '':
          temp += '</li>'
        
        temp += '<li>' + child
      
    return temp
      
  return _recurse_children(value)

The code looks really much cleaner, but would break the current implementation results.

comment:5 Changed 8 years ago by SmileyChris

  • Triage Stage changed from Unreviewed to Ready for checkin

comment:6 Changed 8 years ago by Gary Wilson <gary.wilson@…>

Being able to use this on a simple list will be nice.

comment:7 Changed 8 years ago by Gary Wilson <gary.wilson@…>

Also, the latest patch attached was before simonbun's comments about problems with lists nested more that 2 deep.

comment:8 Changed 8 years ago by Gary Wilson <gary.wilson@…>

  • Needs documentation set
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

Please provide a single patch including a test with simonbun's failing test case. We also might want to officially document that this works for a simple (unnested) list now.

Changed 8 years ago by Dirk Datzert <dummy@…>

I made 1 whole patch, and added the failing testcase

Changed 8 years ago by SmileyChris

completely rewritten with tests and docs

comment:9 Changed 8 years ago by SmileyChris

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

comment:10 Changed 8 years ago by gwilson

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

(In [6019]) Fixed #3184 -- Changed the unordered_list template filter to use a more simple format, while maintaining backwards compatibility with the old format. unordered_list now works with a simple list of items. Thanks for the patch, SmileyChris.

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