advanced-18n: improve error reporting in case of extraction errors

[cboos]

While adding an i18n:msg attribute in Trac, I've not given enough "parameters" to this attribute when expressions were used in the corresponding element. This has lead to a traceback, which is not very nice, as I first thought I've hit a bug in Genshi.

After looking at the code, it appeared that it was simply an error report. This is not OK, extractors should be able to report errors more conveniently (well, I guess that's a Babel issue actually).

Nevertheless here a two suggested improvements:

• the error reporting could benefit from some code refactoring (attachment:refactor-error-reporting-for-extraction-errors-r967.diff​)
• that error shouldn't actually be an error but only a warning, as one can easily produce automatic parameter names (attachment:warn-for-missing-parameters.patch​). That patch applies on top of the former, and shouldn't be committed as such, as it should be better integrated with Babel. Currently the 'babel' logger is only present when Babel is used from the command line, not when called from the setup.py script.

[cboos]

move all the raise IndexError statements in one place

[cboos]

don't raise an IndexError but simply warn about sub-optimal use of the directive

[palgarvio]

Included attachment:refactor-error-reporting-for-extraction-errors-r967.diff​ in [968] Thank You! Nicely done.

Regarding the second patch, I only forced an error to impose good usage of the directive. Either you use it badly and don't use params or you use params and but you provide all of them. Still, logging the error should be cleaner, yes.

I'll see what can be done regarding the logging part of distutils and the stdlib logging.

[cboos]

Replying to palgarvio:

Either you use it badly and don't use params or you use params and but you provide all of them.

In my case, I used i18n:msg="" (is that what you meant with "you use it badly and don't use params"?) but nevertheless I got an error during extraction. More specifically, it was on this change:

trac/versioncontrol/templates/browser.html

@@ -108 +109 @@
 
       <table py:if="properties or file" id="info" summary="Revision info">
         <tr py:if="file">
-          <th scope="col">
+          <th scope="col" i18n:msg="">
             Revision <a href="${href.changeset(rev)}">$rev</a>, ${sizeinfo(file.size)}
             (checked in by ${authorinfo(file.changeset.author)}, ${dateinfo(file.changeset.date)} ago)
           </th>

If this "either/or" behavior is desired, then we could simply do:

genshi/filters/i18n.py

@@ -947 +947 @@
         elif kind is EXPR:
             if self.params:
                 param = self.params.pop(0)
-            else:
+            elif self.orig_params:
                 raise IndexError("'%s' parameters given to 'i18n:%s' but more "
                                  "expressions used in '%s', line %s" % (
                                  ', '.join(self.orig_params),
                                  self.directive.tagname,
                                  os.path.basename(pos[0]), pos[1]))
+            else:
+                param = 'expr%d' % len(self.values)
             self.string.append('%%(%s)s' % param)
             self.events.setdefault(self.stack[-1], []).append(None)
             self.values[param] = (kind, data, pos)

But ideally still without a traceback ;-)

[palgarvio]

Replying to cboos:

Replying to palgarvio:

Either you use it badly and don't use params or you use params and but you provide all of them.

In my case, I used i18n:msg="" (is that what you meant with "you use it badly and don't use params"?) but nevertheless I got an error during extraction. More specifically, it was on this change:

trac/versioncontrol/templates/browser.html

@@ -108 +109 @@
 
       <table py:if="properties or file" id="info" summary="Revision info">
         <tr py:if="file">
-          <th scope="col">
+          <th scope="col" i18n:msg="">
             Revision <a href="${href.changeset(rev)}">$rev</a>, ${sizeinfo(file.size)}
             (checked in by ${authorinfo(file.changeset.author)}, ${dateinfo(file.changeset.date)} ago)
           </th>

If this "either/or" behavior is desired, then we could simply do:

genshi/filters/i18n.py

@@ -947 +947 @@
         elif kind is EXPR:
             if self.params:
                 param = self.params.pop(0)
-            else:
+            elif self.orig_params:
                 raise IndexError("'%s' parameters given to 'i18n:%s' but more "
                                  "expressions used in '%s', line %s" % (
                                  ', '.join(self.orig_params),
                                  self.directive.tagname,
                                  os.path.basename(pos[0]), pos[1]))
+            else:
+                param = 'expr%d' % len(self.values)
             self.string.append('%%(%s)s' % param)
             self.events.setdefault(self.stack[-1], []).append(None)
             self.values[param] = (kind, data, pos)

But ideally still without a traceback ;-)

Hmm, I guess we imposed good usage from the start :) And yes, a traceback is not a good choice :\

K, I'll have a deeper/cleaner look at it.

[palgarvio]

At first thought I think we need to impose good usage because of translations that change the order or words. So, your proposition although good; we get named parameters and only fail if we have the wrong number of them; won't be that helpful for translators. Having expr0, expr1 ain't that helpful for translators since they probably will have to look them up on the source code and most translators are simple users, not coders, so the source code they're seeing might not help at all. Still I'm with you, let's get rid of that traceback :)

[palgarvio]

What would you like to have, exit on this kind of error letting user known what happened or, just log and skip that extraction? I think stopping might be the way:

genshi/filters/i18n.py

@@ -18 +18 @@
 :note: Directives support added since version 0.6
 """
 
+from distutils import log
 from compiler import ast
 from gettext import NullTranslations
 import os
@@ -947 +948 @@
             if self.params:
                 param = self.params.pop(0)
             else:
+                import sys
                 params = ', '.join(['"%s"' % p for p in self.orig_params if p])
                 if params:
                     params = "(%s)" % params
-                raise IndexError("%d parameters%s given to 'i18n:%s' but "
-                                 "%d or more expressions used in '%s', line %s"
-                                 % (len(self.orig_params), params,
-                                    self.directive.tagname,
-                                    len(self.orig_params)+1,
-                                    os.path.basename(pos[0] or
-                                                     'In Memmory Template'),
-                                    pos[1]))
+                log.error("%d parameters%s given to 'i18n:%s' but "
+                          "%d or more expressions used in '%s', line %s" % (
+                          len(self.orig_params), params, self.directive.tagname,
+                          len(self.orig_params)+1,
+                          os.path.basename(pos[0] or 'In Memmory Template'),
+                          pos[1]))
+                sys.exit(1)

Or, if we don't want to rely on distutils.log(I see the above log message, don't know why you had to use stdlib's logging module):

genshi/filters/i18n.py

@@ -947 +947 @@
             if self.params:
                 param = self.params.pop(0)
             else:
+                import sys
                 params = ', '.join(['"%s"' % p for p in self.orig_params if p])
                 if params:
                     params = "(%s)" % params
-                raise IndexError("%d parameters%s given to 'i18n:%s' but "
-                                 "%d or more expressions used in '%s', line %s"
-                                 % (len(self.orig_params), params,
-                                    self.directive.tagname,
-                                    len(self.orig_params)+1,
-                                    os.path.basename(pos[0] or
-                                                     'In Memmory Template'),
-                                    pos[1]))
+                sys.exit("%d parameters%s given to 'i18n:%s' but "
+                         "%d or more expressions used in '%s', line %s. "
+                         "Stopped processing." % (
+                         len(self.orig_params), params, self.directive.tagname,
+                         len(self.orig_params)+1,
+                         os.path.basename(pos[0] or 'In Memmory Template'),
+                         pos[1]))
             self.string.append('%%(%s)s' % param)
             self.events.setdefault(self.stack[-1], []).append(None)
             self.values[param] = (kind, data, pos)

Which one would you prefer?

[cboos]

Neither ;-) I don't think it's up to an extract filter to decide to sys.exit() the program from which it is used. E.g. imagine some kind of web application doing message extraction of uploaded source files (or newly committed files).

I'm OK with the idea of stopping the extraction and presenting this as a kind of fatal error, but I guess we'd need to do something at the Babel level in order to implement this cleanly.

On a related note, using distutils.log works, but I considered using logging.getLogger('babel') because Babel itself set it up in one case: when extraction is performed by the standalone pybabel script (see ​babel/messages/frontend.py). I wonder if that couldn't also done when extraction is done by the setuptools. Also, during extraction itself, some warnings are directly sent to the stderr, see here ​babel/messages/extract.py. So again, this suggests some possible enhancements at the Babel level.

[palgarvio]

Replying to cboos:

Neither ;-) I don't think it's up to an extract filter to decide to sys.exit() the program from which it is used. E.g. imagine some kind of web application doing message extraction of uploaded source files (or newly committed files).

Yep, you're right.

I'm OK with the idea of stopping the extraction and presenting this as a kind of fatal error, but I guess we'd need to do something at the Babel level in order to implement this cleanly.

Well, we could just raise the IndexError's and have babel catch them and log them, extractions would continue but Babel would then show all the extraction errors.

On a related note, using distutils.log works, but I considered using logging.getLogger('babel') because Babel itself set it up in one case: when extraction is performed by the standalone pybabel script (see ​babel/messages/frontend.py). I wonder if that couldn't also done when extraction is done by the setuptools. Also, during extraction itself, some warnings are directly sent to the stderr, see here ​babel/messages/extract.py. So again, this suggests some possible enhancements at the Babel level.

Dunno why we chose to print to sys.stderr on Babel, there should be a reason, can't remember which :)

Distutils is slated to use python's stdlib logging module but that's only for python 3.0. Maybe we can know who is running the code and invoke the proper logger because we should handle both. Again, if we just raise the exception and have Babel catch it, Babel would know where to log it to.

[palgarvio]

The above patch along with ​this babel ticket resolves this issue. As a default Babel set's the error callback to which ever logging facility is being used, distutils or logging but users can provide their own to stop the whole process ignore it or do whatever they want.

This patch depends on the Babel patch on the ticket I said above, which, is not yet included into Babel's trunk.

[palgarvio]

Updated patch.

[palgarvio]

Updated patch to work with the latest branch changes.

[cboos]

Current status is satisfying (r1092), the more advanced error reporting facilities can wait for a future release.

[hodgestar]

Moved to milestone 0.9.