Opened 14 years ago
Last modified 8 years ago
#426 new defect
IndexError in parse_msg
| Reported by: | guillaume@… | Owned by: | hodgestar |
|---|---|---|---|
| Priority: | minor | Milestone: | 0.9 |
| Component: | Internationalization | Version: | 0.6 |
| Keywords: | parse_msg review | Cc: |
Description
I found a problem with the parse_msg version of Genshi 0.6; here's how to reproduce :
from genshi.filters.i18n import parse_msg
parse_msg("""[1:Title text :][2]
This message [3:will] crash $(the_function)s.""")
Traceback (most recent call last):
File "<stdin>", line 1, in ?
File "..../python2.4/site-packages/genshi/filters/i18n.py", line 1143, in parse_msg
parts.append((stack[-1], string))
Attachments (2)
Change History (13)
comment:1 Changed 14 years ago by anonymous
comment:2 Changed 14 years ago by guillaume@…
Sorry, my bad, there was a ':' missing after [2 (it should have been [2:] and not [2] in the translated message).
Still, Genshi should not crash in a situation like this.
comment:3 Changed 14 years ago by hodgestar
I think raising an exception is exactly what Genshi should do in this situation (i.e. receiving a malformed translation message). I agree the current exception is not very helpful. I'm currently leaning towards applying a patch like:
-
genshi/filters/i18n.py
1139 1139 if not stack: 1140 1140 break 1141 1141 1142 if stack != [0]: 1143 raise ValueError("Malformed translated message") 1144 1142 1145 if string: 1143 1146 parts.append((stack[-1], string))
comment:4 Changed 14 years ago by hodgestar
P.S. Obviously it would be better to have a custom exception type and a more helpful error message in the exception but I can add those later if people like the general approach.
comment:5 Changed 14 years ago by cboos
- Component changed from Template processing to Internationalization
- Keywords parse_msg added
Would be great to fix this one, as we see it at times in Trac (#T9171, #T9272, #T10066). The first Trac ticket actually reminded me about the Genshi one #378 ;-)
As for the proposed solution, I believe raising a specific error in parse_msg is fine but only one half of the job. The "end user" code has no way to do anything useful with such an error, otherwise we would already have done something with the IndexError. From the stack trace in #T9171 for example, you can see that (filters aside) the calling Trac code is render_template. At this level, the best we can do is to re-render the whole template a second time with i18n deactivated. Not really appealing. Instead, I would propose that the translate method should catch the exception (or alternatively a return value of None) and give up for that specific translation and use the original string instead (it must therefore gain an additional argument for the original untranslated string).
comment:6 Changed 14 years ago by cboos
- Keywords review added
Please have a look at the t426.diff patch. There I only raise an exception in case there's no adequate fallback (which shouldn't happen with the current use of the API). If you think the API of MessageBuffer can be modified, then I would even suggest to use translate(original, translated), but it's a detail and probably not worth breaking the API.
This shouldn't prevent #378 to be implemented one day, as it would be very handy to catch those errors at the message catalog compilation stage.
comment:7 Changed 14 years ago by cboos
Sorry, got the singular/plural case wrong ... again ;-)
Please only look at next patch, t426.2.diff.
comment:8 follow-up: ↓ 10 Changed 14 years ago by hodgestar
The patch looks good and the test suite passes for me. I'm still somewhat against hiding these errors. Previously a broken translation would be fairly obvious. If the patch is applied broken translations will be much less likely to be spotted. I'm not familiar enough with the translation world to say what developers and users expect to happen though, so I'm happy to bow to the wisdom of others (and commit the patch) if they tell me the norm is to let these errors pass silently.
Note that there are still other trivial cases where a bad translation will break template rendering, e.g.
def test_translate_broken_msg(self):
tmpl = MarkupTemplate("""<html xmlns:py="http://genshi.edgewall.org/"
xmlns:i18n="http://genshi.edgewall.org/i18n">
<p i18n:msg="">
Please see <a href="help.html">Help</a> for <em>details</em>.
</p>
</html>""")
gettext = lambda s: u"Für [2:Details] siehe bitte [3:Hilfe]."
translator = Translator(gettext)
translator.setup(tmpl)
tmpl.generate().render()
I don't think we necessarily have to handle these cases in this ticket but I do think Genshi should ideally behave the same way in both cases (since they're both the result of bad translation messages).
comment:9 Changed 14 years ago by hodgestar
- Owner changed from cmlenz to hodgestar
comment:10 in reply to: ↑ 8 Changed 14 years ago by cboos
Replying to hodgestar:
... Previously a broken translation would be fairly obvious.
Well, as you could see in the Trac tickets referenced in comment:5, the precise source of the errors was not obvious to diagnose. Unless you meant it should be preferable to raise a ValueError containing the problematic message?
If the patch is applied broken translations will be much less likely to be spotted. I'm not familiar enough with the translation world to say what developers and users expect to happen though,
Broken translations will appear as untranslated messages, making them relatively easy to spot. At least the page is still usable so I'm sure the users will be happier. Otherwise in presence of such a problem, the only workaround would be to turn off translations completely (provided the application allows for that).
But really, such problems could and should be detected at compile time (#378).
so I'm happy to bow to the wisdom of others (and commit the patch) if they tell me the norm is to let these errors pass silently.
Indeed, let's also hear about other opinions!
comment:11 Changed 8 years ago by hodgestar
- Milestone changed from 0.7 to 0.9
Moved to milestone 0.9.
The $(the_function) really should have been %(the_function)...
I reduced the test case :
parse_msg("""[1:t][2]t""")