Opened 17 years ago
Closed 12 years ago
#7557 closed New feature (fixed)
ValueError does not catch all cases when trying to determine type of variable in template.Variable class
| Reported by: | Tobias McNulty | Owned by: | Steffen Zieger |
|---|---|---|---|
| Component: | Template system | Version: | dev |
| Severity: | Normal | Keywords: | |
| Cc: | bmispelon@…, Yohann Gabory, me@… | Triage Stage: | Accepted |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | yes | UI/UX: | no |
Description
File "/usr/lib/python2.5/site-packages/caktus/django/templatetags/__init__.py", line 59, in __init__
self.kwargs = dict([(k, Variable(arg)) for k, arg in kwargs.items()])
File "/usr/lib/python2.5/site-packages/django/template/__init__.py", line 628, in __init__
self.literal = float(var)
TypeError: float() argument must be a string or a number
In my case "var" was a dict. obviously dict is not a supported variable type, so django should raise a more obvious error here.
Attachments (3)
Change History (17)
comment:1 by , 17 years ago
comment:2 by , 17 years ago
Sorry, here is how to reproduce it:
from django.template import Variable
Variable({})
by , 17 years ago
| Attachment: | 7557-1.diff added |
|---|
comment:3 by , 17 years ago
| Has patch: | set |
|---|---|
| Triage Stage: | Unreviewed → Design decision needed |
TypeError makes sense, just not on the float call. Patch to re-raise it with a more descriptive error.
comment:4 by , 15 years ago
| Severity: | → Normal |
|---|---|
| Type: | → New feature |
comment:5 by , 14 years ago
| Easy pickings: | unset |
|---|---|
| Triage Stage: | Design decision needed → Accepted |
| UI/UX: | unset |
Marking as accepted, but this patch is wrong, Variable's constructor takes a basestring, and nothing else, we should just typecheck the input IMO.
comment:6 by , 13 years ago
| Cc: | added |
|---|---|
| Easy pickings: | set |
| Needs tests: | set |
| Patch needs improvement: | set |
comment:8 by , 13 years ago
This patch is better, but to ensure python3 compatibility, you need to use six.string_types instead of basestring.
You also need to add some tests for this new feature (maybe in tests/template_tests/test_parser.py seeing as there's already some tests for Variable in there).
Thanks.
by , 13 years ago
| Attachment: | type_check.diff added |
|---|
use six.string_type instead of basestring for python3 compatibility. Added a test to check that a non string value raise a TypeError
comment:9 by , 12 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
by , 12 years ago
| Attachment: | 7557_type_check_test_working.diff added |
|---|
Fixes the test and improves code
comment:10 by , 12 years ago
| Cc: | added |
|---|
comment:11 by , 12 years ago
| Needs tests: | unset |
|---|---|
| Patch needs improvement: | unset |
comment:12 by , 12 years ago
@saz : your attachment "type_check_test_working" is a copy'n'past of "type_check". For what purpose did you made it ?
comment:13 by , 12 years ago
Almost. Have a look at line 25 in your patch and lines 25 to 27 in my patch.
After applying your patch, running the tests has thrown an exception for me.
Changing the self.assertRaisses line as it is documented (http://docs.python.org/2/library/unittest.html#unittest.TestCase.assertRaises) everything is working as expected.
comment:14 by , 12 years ago
| Resolution: | → fixed |
|---|---|
| Status: | assigned → closed |
It's not clear from the report what you are doing to trigger this error. Without knowing that, it's difficult to choose from amongst possible solutions.