#11687 closed (fixed)
The 'add' template filter only works for integers, and can fail noisily
Reported by: | Bradley Ayers | Owned by: | Filip Gruszczyński |
---|---|---|---|
Component: | Template system | Version: | 1.1 |
Severity: | Keywords: | ||
Cc: | 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
The 'add' template tag attempts to convert both arguments to ints using int() and there's no try/except around it (thus making it noisy if you try and add two objects which are not ints). In addition, there's no reason why it should be limited to only adding together integers, it should be able to support any two objects that can be added together.
Attachments (1)
Change History (23)
comment:1 by , 15 years ago
comment:2 by , 15 years ago
Maybe something like this:
def add(value, arg): """Adds the arg to the value.""" if isintnace(value, string): return value + arg elif isinstance(value, int): try: return value + int(arg) except TypeError: return value elif isinstance(value, list): try: return value + list(arg.split(',')) except AttributeError: return value elif isinstance(value, tuple): try: return value + tuple(arg.split(',')) except AttributeError: return value return value
Of course it can be extended a little more.
comment:4 by , 15 years ago
I think better not convert types at all (explicit is better than implicit) and use duck typing.
def add(value, arg): try: return value + arg except TypeError: return value
This will work for any object with __add__
or __radd__
method.
comment:5 by , 15 years ago
This won't work with ints, because value is either an int (and will raise an exception) or a string (and it will concatenate those string).
Furthermore it couldn't be used in following situtation:
x = [] ... x|add:'5,6,7'
which for some could be useful.
comment:6 by , 15 years ago
Owner: | changed from | to
---|
comment:7 by , 15 years ago
OK, I don't know, whether add should have any more functionality, but it certainly needs to be silenced and proper tests should be written. I am posting a patch with just that.
comment:8 by , 15 years ago
Has patch: | set |
---|
comment:9 by , 15 years ago
Triage Stage: | Unreviewed → Design decision needed |
---|
I agree with dc; don't worry with a load of extra functionality (but IMO it should handle non-ints).
The try/except may as well be a catch-all, too, rather than TypeError
or ValueError
specific.
Hrm... to be totally backwards compatible, I guess that we'd need to continue with the current behaviour of trying to cast to int first (in case people are relying on adding string-integers), then fall back to a ducktype add.
That's pretty annoying if you wanted to add floats/decimals though.
I'm going to push to a design decision regarding which direction to go. Bring it up on the google dev group if you want some more discussion.
comment:10 by , 15 years ago
Non-int adding would be useful for #11689 - if the non-int part of this ticket is accepted, that can be closed as a duplicate.
comment:11 by , 15 years ago
I don't like sucking all exceptions and this shouldn't be done. Imagine there is a custom object, that can be cast to an int and during coercing it will raise a different exception. Why should user lose this information? This is completely contrary to python zen.
When it comes to ducktyping, you have to remember, that the object on which filter is used, will have to able to accept a string as an argument of add. Most basic types:
In [1]: [] + '' --------------------------------------------------------------------------- TypeError Traceback (most recent call last) /home/gruszczy/<ipython console> in <module>() TypeError: can only concatenate list (not "str") to list In [2]: () + '' --------------------------------------------------------------------------- TypeError Traceback (most recent call last) /home/gruszczy/<ipython console> in <module>() TypeError: can only concatenate tuple (not "str") to tuple
just can't do this.
comment:12 by , 15 years ago
The user should lose this information because a filter should never raise an exception.
Read the first paragraphs here: http://docs.djangoproject.com/en/dev/howto/custom-template-tags/#writing-custom-template-filters
I understand ducktyping.
A filter doesn't only accept strings as an argument, it could be any object type. If they can't be added together, then it should just fail silently - that's the template author's problem. This is not javascript, so no automatic casting.
comment:13 by , 15 years ago
If they should never raise an exception, then indeed it should be sucking all exceptions.
The filter obviously accepts all objects, because that's just how Python works. However, if used in template only strings will be passed into such filter. Unless object on which filter is run can somehow use string in add, you won't get anything useful. Neither integers, nor lists/tuples can do that. Could you give some example of objects from stdlib/django that could benefit from using add with a string, that aren't strings?
comment:14 by , 15 years ago
I'm not sure where your delusion comes from that a template only deals with strings. I already gave a perfect example of a real life useful non-string addition in a template: see ticket #11689.
Here's another arbitrary example:
{% for obj in obj.list_a|add:obj2.list_b %}...{% endfor %}
comment:15 by , 15 years ago
It was truly a delusion, because I had no idea that filter could work like this. If so, I'll provide proper patch and tests for a few different cases.
comment:16 by , 15 years ago
You may have had that view because Django templates always applie unicode()
to the final output of variable tags. Glad we got to the conclusion in the end :)
I'll review your patch when it comes in and push forwards if it's all good. Remember, we need to keep the current behaviour so it should look something like this:
try: value, arg = int(value), int(arg) except (TypeError, ValueError): pass try: return value + arg except: return value
comment:17 by , 15 years ago
Yup, I know. The code itself is rather short, I just need to write some more tests to keep it safe. There is only one thing that isn't very nice - if someone wants to concatenate '123' and '456', he will get 579, which might not be exactly the thing he wants. I'll write the docs too.
by , 15 years ago
Attachment: | add_filter.diff added |
---|
coercing to int, then ducktyping + tests + docs
comment:21 by , 15 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
You mean 'add' filter, not the tag right?
Allowing any two objects to be added together isn't that easy. If you use this filter on an object that can be sumed with an int, the filter must somehow know to coerce the argument to int. And what if you want to add a string to a string? Then no coercion should be be applied. And if there was a list or tuple it would have to be turned into one.
Maybe some simpler filter, that would accept some several base types (int, string, list, tuple, set, dict) and allowing adding one to another. If it couldn't turn the argument into something acceptable, it would fail silently and return object on which filter was applied.