Django

Code

Ticket #7027 (closed: fixed)

Opened 7 months ago

Last modified 3 months ago

Translating string constans passed to template tags breaks if the string contains spaces

Reported by: Konstantinos Metaxas <kmetaxas@gmail.com> Assigned to: jacob
Milestone: 1.0 Component: Internationalization
Version: SVN Keywords:
Cc: Triage Stage: Accepted
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 0

Description

Hi. The documentation for i18n states that the following can be used to translate string constants passed to template tags: 

{% some_special_tag _("Page not found") value|yesno:_("yes,no") %}

However, when i use it with a string that contains spaces (like in the example in the docs) it raises a TemplateSyntaxError? claiming that more parameters where passed to the some_special_tag than it accepts.

It seems that django.utils.text.smart_split breaks up quoted strings that are enclosed like thus: _("my quoted string") and contain spaces. Example: 

>>> from django.utils.text import smart_split
>>> list(smart_split(' _("my quoted string") '))
[u'_("my', u'quoted', u'string")']
#Without spaces
>>> list(smart_split(' _("myquotedstring") '))
[u'_("myquotedstring")']

This makes translating multi-word string constants in template tags impossible. I am using svn revision 7425.

Attachments

fix_tag_translation.diff (3.1 kB) - added by mrts on 06/18/08 08:46:50.
fix_tag_translation-refactored-re_7027.diff (4.5 kB) - added by mrts on 08/27/08 13:15:46.
Refactored the regex and wrote an explanation
fix_tag_translation-refactored-re-strict_gettext_matching_7027.diff (4.5 kB) - added by mrts on 08/27/08 13:35:56.
This one won't let " ") and \(" " through, IMHO not worth the extra complexity though
fix_tag_translation-refactored-re-allow_empty_strings-7027.diff (4.5 kB) - added by mrts on 08/28/08 13:58:15.
Allow '' and ""

Change History

04/15/08 22:17:59 changed by mtredinnick

  • needs_better_patch changed.
  • stage changed from Unreviewed to Accepted.
  • component changed from Template system to Internationalization.
  • needs_tests changed.
  • needs_docs changed.

06/18/08 08:46:16 changed by mrts

  • has_patch set to 1.
  • milestone set to 1.0.

This clearly a bug that needs to fixed before 1.0. Attaching a patch with fixes and tests.

06/18/08 08:46:50 changed by mrts

  • attachment fix_tag_translation.diff added.

06/19/08 08:56:39 changed by mrts

Just a note that the patch passes all tests.

08/26/08 16:39:05 changed by jacob

Goddamn that regex is scary. mrts, think you can rewrite it to use re.X and add some comments so we know what's going on there?

08/27/08 10:05:55 changed by mrts

Will do it shortly.

08/27/08 10:28:09 changed by mrts

But note that basically I just circumfixed non-grouping matches for either _('') or _("") and used raw strings to lessen the wild escaping in the original.

08/27/08 10:51:44 changed by mrts

Now that I review it, it looks like the original pattern (not written by me and carried over in this patch) is too complex. I should have cut it more radically. The proliferation of \ had a bad smell but I kind of assumed the person knew what he was doing. Jacob, do you know who it was so that I can discuss this with him? I see a much more straightforward pattern, but perhaps I'm missing something.

08/27/08 12:14:39 changed by mtredinnick

Just upload a new patch. We'll work out if it's good enough. But, really, at this point, the smallest possible change that gets the job done is the only correct approach. Larger changes contain more bugs.

08/27/08 13:15:46 changed by mrts

  • attachment fix_tag_translation-refactored-re_7027.diff added.

Refactored the regex and wrote an explanation

08/27/08 13:30:43 changed by mrts

Just a note -- this passes all tests.

08/27/08 13:35:56 changed by mrts

  • attachment fix_tag_translation-refactored-re-strict_gettext_matching_7027.diff added.

This one won't let " ") and \(" " through, IMHO not worth the extra complexity though

08/27/08 14:49:53 changed by anonymous

This one won't let " ") and \(" " through, IMHO not worth the extra complexity though -- entirely pointless. Group C (see comments) will match unclosed bits anyway, so the last patch isn't really useful, there's no difference between _("foo and _("foo bar" correctness-wise -- both are invalid. Thus, fix_tag_translation-refactored-re_7027.diff is the way to go.

08/27/08 14:57:22 changed by mrts

And here are some illustrations for the invalid cases: 

# with fix_tag_translation-refactored-re-strict_gettext_matching_7027.diff
>>> list(smart_split("_('foo bar'"))
[u"_('foo", u"bar'"]
>>> list(smart_split("_('foo bar"))
[u"_('foo", u'bar']
>>> list(smart_split("'foo bar"))
[u"'foo", u'bar']

# with fix_tag_translation-refactored-re_7027.diff -- best behaviour
>>> list(smart_split("_('foo bar'"))
[u"_('foo bar'"]
>>> list(smart_split("_('foo bar"))
[u"_('foo", u'bar']
>>> list(smart_split("'foo bar"))
[u"'foo", u'bar']

# original 
>>> list(smart_split("_('foo bar'"))
[u"_('foo", u"bar'"]
>>> list(smart_split("_('foo bar"))
[u"_('foo", u'bar']
>>> list(smart_split("'foo bar"))
[u"'foo", u'bar']

08/28/08 13:52:05 changed by mrts

An afterthought: the regex should allow empty strings, so (?:[^"\\]|\\.)+ should be (?:[^"\\]|\\.)* instead.

08/28/08 13:58:15 changed by mrts

  • attachment fix_tag_translation-refactored-re-allow_empty_strings-7027.diff added.

Allow '' and ""

08/31/08 12:56:05 changed by jacob

  • owner changed from nobody to jacob.
  • status changed from new to assigned.

After thinking this through a bit more, doing this is smart_split isn't a good idea: smart_split shouldn't have to have any idea about translation-marked strings. I think the right place to this is in template.Token.split_contents, so I'll have a whack at doing it there.

08/31/08 13:28:07 changed by jacob

  • status changed from assigned to closed.
  • resolution set to fixed.

(In [8769]) Fixed #7027: template tags now corectly break tokens around strings marked for translation.

Add/Change #7027 (Translating string constans passed to template tags breaks if the string contains spaces)




Change Properties
Action