Opened 11 years ago

Closed 5 years ago

Last modified 5 years ago

#20122 closed Cleanup/optimization (fixed)

Pluralize filter sometimes returns singular form instead of an empty string for invalid inputs

Reported by: Aymeric Augustin Owned by: Tobias Kunze
Component: Template system Version: dev
Severity: Normal Keywords:
Cc: Jorge C. Leitão Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Filters are documented to return either their input unchanged, or the empty string, whatever makes most sense, when they're used incorrectly. The pluralize filter returns the empty string in such cases, for instance when it receives more than two forms (singular, plural).

However, it returns the singular form instead of the empty string when it's passed an object that isn't a number, a string or a list.

Failing test case:

--- a/tests/defaultfilters/tests.py
+++ b/tests/defaultfilters/tests.py
@@ -622,6 +622,9 @@ class DefaultFiltersTests(TestCase):
         self.assertEqual(pluralize(2,'y,ies'), 'ies')
         self.assertEqual(pluralize(0,'y,ies,error'), '')
 
+    def test_pluralize_error(self):
+        self.assertEqual(pluralize(object, 'y,ies'), '')
+
     def test_phone2numeric(self):
         self.assertEqual(phone2numeric_filter('0800 flowers'), '0800 3569377')
 

I understand that the implementation is crafted to avoid isinstance checks, but in this case we really want different logic depending on the type of the input. I think the filter should be rewritten with the following pseudo-code:

if the value is a number:
    return singular if value is 1 else plural
if the value is a string:
    return singular if value is '1' else plural
if the value has a length (needs a try/except TypeError):
    return singular if length is 1 else plural
return ''

I discovered this while working on #16723.

Attachments (1)

defaultfilters_fix_pluralize.diff (1.8 KB ) - added by Jorge C. Leitão 11 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 by Claude Paroz, 11 years ago

Triage Stage: UnreviewedAccepted

comment:2 by Jorge C. Leitão, 11 years ago

I don't agree with the suggested implementation because when you pass a string to a pluralize it should not assume the string can be "pluralized". In your case, if you pass 'one', it would return the plural, and I think that it should return an empty string: it is language dependent whether strings represent singular or plural number.

One possible solution that compromises both cases:

if value == 1 or value == u'1':
    return singular
try:
    float(value)
    return plural
except ValueError:
   # a string that cannot be parsed as a float and thus is an undefined number 
   return ''

This avoids the isinstance() and also corrects the pluralization of floats (1 unit but 1.1 units).

Cheers,
Jorge

comment:3 by Jorge C. Leitão, 11 years ago

Cc: Jorge C. Leitão added
Owner: changed from nobody to Jorge C. Leitão
Status: newassigned

comment:4 by Jorge C. Leitão, 11 years ago

The added patch provides the following functionality:

if value is 1 or '1' returns singular
elif it is a string representable by a float returns plural
elif is something that can be argument of len():
   if len(value) == 1: returns singular
   else: returns plural
else, returns ''

This complements the existing functionality, extends to float numbers, and fixes this ticket, returning an empty string in some case where a number (plural/singular) cannot be defined by the value.

Last edited 11 years ago by Jorge C. Leitão (previous) (diff)

by Jorge C. Leitão, 11 years ago

comment:5 by Jorge C. Leitão, 11 years ago

Has patch: set

comment:6 by Łukasz Rekucki, 11 years ago

Needs tests: set
Patch needs improvement: set

comment:7 by Tobias Kunze, 5 years ago

Needs tests: unset
Owner: changed from Jorge C. Leitão to Tobias Kunze

comment:8 by Tobias Kunze, 5 years ago

Patch needs improvement: unset

comment:9 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In e3968df5:

Refs #20122 -- Corrected documentation of pluralize template filter.

comment:10 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In e6836136:

Fixed #20122 -- Made pluralize template filter return on invalid input.

comment:11 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In bf9e0e34:

[2.2.x] Refs #20122 -- Corrected documentation of pluralize template filter.

Backport of e3968df527c4d378677f4784fb1bc0c86950fcf8 from master

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