Merge lp:~rachidbm/ubuntu-l10n-tools/search into lp:ubuntu-l10n-tools

[Rachid]

 - Search tool: Added some error checking in (removed TODO's)
 - Search tool: Implemented search on 'both' mode (translation and original)
 - In general: Fixed launchpadmanager and added some DEBUG messages

[David Planella (dpm)]

Hi Rachid,

Thanks a lot for the branch!

A couple of things I've noticed while reviewing it:

* On setup.py you add "print >> sys.stderr, 'To build ul10n-tools you need https://launchpad.net/python-distutils-extra : \nsudo apt-get install python-distutils-extra python-polib'". While it is true, I don't think it's the best way to check it here, as the exception only checks for python-distutils-extra and shouldn't have anything to do with python-polib, which is not a build dependency, but a runtime dependency. I think having the check on the file that imports polib should be enough

* In ul10n_tools/lp_get_templates/__init__.py you add "DISTRO_CODENAMES = ('hardy', 'intrepid', 'jaunty', 'karmic', 'lucid', 'maverick', 'natty')", which is correct, but we should probably be more dynamic and use launchpadlib to get the releases. Nothing to change here, just a mental note to myself that we should check out the capabilites of the Launchpad API concerning templates nowadays, to see if we can get rid of the screen-scraping :)

* This is optional but you are using correct constructs such as "Source package: %s" % (options.source_package), but I'd recommend using "Source package: {0}".format(options.source_package) (See the str.format method on http://docs.python.org/library/stdtypes.html#string-methods: "This method of string formatting is the new standard in Python 3.0, and should be preferred to the % formatting [...]")

* In the future, if you submit a branch for each feature (i.e. each change you make for a tool), it will make it much easier to review (no need to do it for this now, I'll just review the current branch).

* On ul10n_tools/lp_set_pot_priority/__init__.py and ul10n_tools/search/TranslationsSearch.py you are adding some code that is commented out. If it's not used, could you please remove it?

Do you think you could have a look at these points and re-submit the merge proposal before merging?

Thanks!

review: Needs Fixing

[David Planella (dpm)]

A couple of other things I've noticed (I'm still reviewing the 'search both' implementation):

* On ul10n_tools/search/TranslationsSearch.py, line 336 of the diff, please don't reformat the code. In general, when submitting merge proposals the changes should concentrate on the feature, so that the diff contains as little noise as possible.

* On ul10n_tools/search/TranslationsSearch.py, line 333, one thing that I really like is that you've managed to unify the code to have the same search loop for plural and singular entries (the "for entry_to_search in entries_to_search:" part. I think it might be interesting to explore optimizing this further and rewrite the for loop in a generic way that would work for both plural and singular, and have it called just once after the if clause in which we detect if the entry is plural or singular.

What do you think?

[Rachid (rachidbm)]

Hi David,

On Mon, May 30, 2011 at 6:08 PM, David Planella
<email address hidden>wrote:

> Review: Needs Fixing
> Hi Rachid,
>
> Thanks a lot for the branch!
>
> A couple of things I've noticed while reviewing it:
>
> * On setup.py you add "print >> sys.stderr, 'To build ul10n-tools you need
> https://launchpad.net/python-distutils-extra : \nsudo apt-get install
> python-distutils-extra python-polib'". While it is true, I don't think it's
> the best way to check it here, as the exception only checks for
> python-distutils-extra and shouldn't have anything to do with python-polib,
> which is not a build dependency, but a runtime dependency. I think having
> the check on the file that imports polib should be enough
>
OK, so I should only remove python-polib? Or the whole apt-get line? (I
doubt because apt-get is Ubuntu/Debian specific)

>
> * In ul10n_tools/lp_get_templates/__init__.py you add "DISTRO_CODENAMES =
> ('hardy', 'intrepid', 'jaunty', 'karmic', 'lucid', 'maverick', 'natty')",
> which is correct, but we should probably be more dynamic and use
> launchpadlib to get the releases. Nothing to change here, just a mental note
> to myself that we should check out the capabilites of the Launchpad API
> concerning templates nowadays, to see if we can get rid of the
> screen-scraping :)
>
I was thinking about handling the DISTRO_CODENAMES in a central place
anyway. Didn't thought about the Launchpad API, but I shall investigate that
later.

>
> * This is optional but you are using correct constructs such as "Source
> package: %s" % (options.source_package), but I'd recommend using "Source
> package: {0}".format(options.source_package) (See the str.format method on
> http://docs.python.org/library/stdtypes.html#string-methods: "This method
> of string formatting is the new standard in Python 3.0, and should be
> preferred to the % formatting [...]")
>
I'm not a seasoned Python programmer, thanks for this tip ;)

>
> * In the future, if you submit a branch for each feature (i.e. each change
> you make for a tool), it will make it much easier to review (no need to do
> it for this now, I'll just review the current branch).
>
OK

>
> * On ul10n_tools/lp_set_pot_priority/__init__.py and
> ul10n_tools/search/TranslationsSearch.py you are adding some code that is
> commented out. If it's not used, could you please remove it?
>
Which lines exactly? Do you mean the "Currently dead code" remark?

>
> Do you think you could have a look at these points and re-submit the merge
> proposal before merging?
>
> Thanks!
> --
> https://code.launchpad.net/~rachidbm/ubuntu-l10n-tools/search/+merge/62857
> You are the owner of lp:~rachidbm/ubuntu-l10n-tools/search.
>

[Rachid (rachidbm)]

On Mon, May 30, 2011 at 6:35 PM, David Planella
<email address hidden>wrote:

> A couple of other things I've noticed (I'm still reviewing the 'search
> both' implementation):
>
> * On ul10n_tools/search/TranslationsSearch.py, line 336 of the diff, please
> don't reformat the code. In general, when submitting merge proposals the
> changes should concentrate on the feature, so that the diff contains as
> little noise as possible.
>
I understand that this is not desirable for the reviewer (and now bazaar
thinks I changed that code). I'll try to check the diff before committing.
Tips on this procedure are welcome.

> * On ul10n_tools/search/TranslationsSearch.py, line 333, one thing that I
> really like is that you've managed to unify the code to have the same search
> loop for plural and singular entries (the "for entry_to_search in
> entries_to_search:" part. I think it might be interesting to explore
> optimizing this further and rewrite the for loop in a generic way that would
> work for both plural and singular, and have it called just once after the if
> clause in which we detect if the entry is plural or singular.
>
Thanks and good point. I think it can be done relatively easy by changing
the __add_match()
Just give it the entire entry object to add_match and do the check for
plural there (if that's still needed though).
By looking to the code I think that just giving the entry to __add_match is
enough. But I'll find that out.

>
> What do you think?
> --
> https://code.launchpad.net/~rachidbm/ubuntu-l10n-tools/search/+merge/62857
> You are the owner of lp:~rachidbm/ubuntu-l10n-tools/search.
>

[David Planella (dpm)]

El dl 30 de 05 de 2011 a les 20:19 +0000, en/na Rachid va escriure:
> Hi David,
>
> On Mon, May 30, 2011 at 6:08 PM, David Planella
> <email address hidden>wrote:
>
> > Review: Needs Fixing
> > Hi Rachid,
> >
> > Thanks a lot for the branch!
> >
> > A couple of things I've noticed while reviewing it:
> >
> > * On setup.py you add "print >> sys.stderr, 'To build ul10n-tools you need
> > https://launchpad.net/python-distutils-extra : \nsudo apt-get install
> > python-distutils-extra python-polib'". While it is true, I don't think it's
> > the best way to check it here, as the exception only checks for
> > python-distutils-extra and shouldn't have anything to do with python-polib,
> > which is not a build dependency, but a runtime dependency. I think having
> > the check on the file that imports polib should be enough
> >
> OK, so I should only remove python-polib? Or the whole apt-get line? (I
> doubt because apt-get is Ubuntu/Debian specific)
>

I'd suggest removing the python-polib reference for now. I personally
wouldn't have put the apt-line there, since this makes the tool even
more Ubuntu-specific, but as we're just targetting Ubuntu for now, it
can stay if you want.

>
> >
> > * In ul10n_tools/lp_get_templates/__init__.py you add "DISTRO_CODENAMES =
> > ('hardy', 'intrepid', 'jaunty', 'karmic', 'lucid', 'maverick', 'natty')",
> > which is correct, but we should probably be more dynamic and use
> > launchpadlib to get the releases. Nothing to change here, just a mental note
> > to myself that we should check out the capabilites of the Launchpad API
> > concerning templates nowadays, to see if we can get rid of the
> > screen-scraping :)
> >
> I was thinking about handling the DISTRO_CODENAMES in a central place
> anyway. Didn't thought about the Launchpad API, but I shall investigate that
> later.
>

Yeah, good idea, perhaps just as a get_ubuntu_releases() function in
launchpadmanager.py that returns a list of active distro releases. IIRC
there is already some code in one of the tools that returns releases
from Launchpad (or at least the current release). Anyway, this was only
an idea, not directly related with this merge proposal.

>
> >
> > * This is optional but you are using correct constructs such as "Source
> > package: %s" % (options.source_package), but I'd recommend using "Source
> > package: {0}".format(options.source_package) (See the str.format method on
> > http://docs.python.org/library/stdtypes.html#string-methods: "This method
> > of string formatting is the new standard in Python 3.0, and should be
> > preferred to the % formatting [...]")
> >
> I'm not a seasoned Python programmer, thanks for this tip ;)
>
> >
> > * In the future, if you submit a branch for each feature (i.e. each change
> > you make for a tool), it will make it much easier to review (no need to do
> > it for this now, I'll just review the current branch).
> >
> OK
>
> >
> > * On ul10n_tools/lp_set_pot_priority/__init__.py and
> > ul10n_tools/search/TranslationsSearch.py you are adding some code that is
> > commented out. If it's not used, could you please remove it?
> >
> Which lines exactly? Do you mean the "Currently dead code" remark?
>...

[lp:~rachidbm/ubuntu-l10n-tools/search]

7. By David Planella on 2011-05-31

Modified lp_get_queue tool to return all queue entries now, in CSV format

8. By David Planella on 2011-05-31

Merged in changes from Rachid BM. Thanks!
 - Search tool: Added some error checking in (removed TODO's)
 - Search tool: Implemented search on 'both' mode (translation and original)
 - Search tool: Generalized for loop such that it's the same for plural and singular
 - In general: Fixed launchpadmanager and added some DEBUG messages

9. By David Planella on 2011-05-31

Added some todos and minor cleanups for the priority setter tool

10. By David Planella on 2011-06-08

Merged rachidbm's changes for the search tool

 - Added support for case-insensitive search
 - Added comment in HELP text that regex can be used

11. By David Planella on 2011-06-08

Merged in rachidbm's changes:

- get-team-info: minor refactoring

12. By David Planella on 2011-06-08

Minor fixes and comments in the priority setter tool

13. By David Planella on 2011-06-08

Priority setter: used the csv module to read the csv file

14. By David Planella on 2011-06-20

Merged changes from Rachid BM:

ul10n-lp-set-priority tool
* Retrieve active series from Launchpad
* Added class to handle the stuff regarding POT and Launchpad
* Prioritise just one template or give an CSV file
* Edit 1 pot over several distro codenames
* Removed unnecessary has_header check in CSV reader
* Removed obsolete imports

15. By Rachid on 2011-08-15

Added parameter (-p) to print only package names

Unmerged revisions

15. By Rachid on 2011-08-15

Added parameter (-p) to print only package names

=== modified file 'README'
--- README	2011-05-26 11:08:04 +0000
+++ README	2011-05-31 08:16:39 +0000
@@ -14,7 +14,7 @@
 ul10n-lp-get-imports-info
 -------------------------
 
-Returns internal Launchpad URLs for translation tarball imports. Useful when we need to check a translations import when there are problems. This way we ban just download it from the URL without the need of locally building a package.
+Returns internal Launchpad URLs for translation tarball imports. Useful when we need to check a translations import when there are problems. This way we can just download it from the URL without the need of locally building a package.
 
 * Status: not tested since it was moved to ul10n_tools, might need some tweaking.
 

=== modified file 'bin/ul10n-lp-set-pot-priority'
--- bin/ul10n-lp-set-pot-priority	2011-05-30 08:41:38 +0000
+++ bin/ul10n-lp-set-pot-priority	2011-05-31 08:16:39 +0000
@@ -15,7 +15,8 @@
     and PROJECT_ROOT_DIRECTORY not in sys.path):
     sys.path.insert(0, PROJECT_ROOT_DIRECTORY)
     os.putenv('PYTHONPATH', PROJECT_ROOT_DIRECTORY) # for subprocesses
-
+ 
 import ul10n_tools.lp_set_pot_priority as lp_set_pot_priority
 
-lp_set_pot_priority.main()
+lp_set_pot_priority.main() 
+

=== modified file 'setup.py'
--- setup.py	2011-04-09 08:51:51 +0000
+++ setup.py	2011-05-31 08:16:39 +0000
@@ -12,7 +12,7 @@
 try:
     import DistUtilsExtra.auto
 except ImportError:
-    print >> sys.stderr, 'To build ul10n-tools you need https://launchpad.net/python-distutils-extra'
+    print >> sys.stderr, 'To build ul10n-tools you need https://launchpad.net/python-distutils-extra : \nsudo apt-get install python-distutils-extra'
     sys.exit(1)
 assert DistUtilsExtra.auto.__version__ >= '2.18', 'needs DistUtilsExtra.auto >= 2.18'
 

=== modified file 'ul10n_tools/__init__.py' (properties changed: -x to +x)
=== modified file 'ul10n_tools/lp_get_imports_info/__init__.py'
--- ul10n_tools/lp_get_imports_info/__init__.py	2011-05-30 08:41:38 +0000
+++ ul10n_tools/lp_get_imports_info/__init__.py	2011-05-31 08:16:39 +0000
@@ -21,6 +21,7 @@
 import logging
 import optparse
 from ul10n_tools import ul10n_toolsconfig
+from ul10n_tools.utils.launchpadmanager import get_launchpad
 
 LEVELS = (  logging.ERROR,
             logging.WARNING,
@@ -28,8 +29,6 @@
             logging.DEBUG,
             )
 
-from ul10n_tools.utils.launchpadmanager import get_launchpad
-
 DISTRO_CODENAMES = ('hardy', 'intrepid', 'jaunty', 'karmic', 'lucid', 'maverick')
 
 
@@ -61,9 +60,9 @@
         parser.print_help()
         print "\nYou must specify a distro and a date."
     else:
-        lp = get_launchpad()
+        launchpad = get_launchpad()
 
-        distrorelease = lp.distributions['ubuntu'].getSeries(name_or_version = distro_codename)
+        distrorelease = launchpad.distributions['ubuntu'].getSeries(name_or_version = distro_codename)
 
         for p in distrorelease.getPackageUploads(created_since_date = upload_date,
             custom_type = tarball_type):

=== modified file 'ul10n_tools/lp_get_queue/__init__.py'
--- ul10n_tools/lp_get_queue/__init__.py	2011-05-30 08:41:38 +0000
+++ ul10n_tools/lp_get_queue/__init__.py	2011-05-31 08:16:39 +0000
@@ -21,6 +21,7 @@
 import logging
 import optparse
 from ul10n_tools import ul10n_toolsconfig
+from ul10n_tools.utils.launchpadmanager import get_launchpad
 
 LEVELS = (  logging.ERROR,
             logging.WARNING,
@@ -28,8 +29,6 @@
             logging.DEBUG,
             )
 
-from ul10n_tools.utils.launchpadmanager import get_launchpad
-
 DISTRO_CODENAMES = ('hardy', 'intrepid', 'jaunty', 'karmic', 'lucid', 'maverick')
 
 
@@ -49,13 +48,13 @@
 
     distro_codename = options.distro_codename
 
-    lp = get_launchpad()
+    launchpad = get_launchpad()
 
     if distro_codename is None:
         ubuntu = launchpad.distributions["ubuntu"]
         distrorelease = ubuntu.current_series
     else:
-        distrorelease = lp.distributions['ubuntu'].getSeries(name_or_version = distro_codename)
+        distrorelease = launchpad.distributions['ubuntu'].getSeries(name_or_version = distro_codename)
 
     entries = distrorelease.getTranslationImportQueueEntries(file_extension = '.pot',
                                                              import_status='Needs Review')[1:10]

=== modified file 'ul10n_tools/lp_get_team_info/__init__.py'
--- ul10n_tools/lp_get_team_info/__init__.py	2011-05-20 11:55:08 +0000
+++ ul10n_tools/lp_get_team_info/__init__.py	2011-05-31 08:16:39 +0000
@@ -26,7 +26,7 @@
 import tempfile
 import atexit
 import shutil
-from ul10n_tools.launchpadmanager import get_launchpad
+from ul10n_tools.utils.launchpadmanager import get_launchpad
 import csv
 import urllib2
 import re

=== modified file 'ul10n_tools/lp_get_templates/__init__.py'
--- ul10n_tools/lp_get_templates/__init__.py	2011-05-20 11:55:08 +0000
+++ ul10n_tools/lp_get_templates/__init__.py	2011-05-31 08:16:39 +0000
@@ -35,7 +35,7 @@
 LP_TRANSLATIONS_PAGE = "https://translations.launchpad.net/%s/%s/+source/%s"
 RE_TEMPLATE = '.*<a href="/%s/%s/\+source/%s/\+pots/[a-zA-Z0-9\-\.]*"'
 DISTRO_ID = 'ubuntu'
-DISTRO_CODENAMES = ('hardy', 'intrepid', 'jaunty', 'karmic', 'lucid')
+DISTRO_CODENAMES = ('hardy', 'intrepid', 'jaunty', 'karmic', 'lucid', 'maverick', 'natty')
 
 class Ignore404ErrorHandler(urllib2.HTTPDefaultErrorHandler):
     """ Error handler which does not raise a HTTPError if a page is not
@@ -59,7 +59,10 @@
     templates = list()
 
     opener = urllib2.build_opener(Ignore404ErrorHandler)
-    page = opener.open(LP_TRANSLATIONS_PAGE % (distro_id, distro_codename, source_package))
+    URL_TEMPLATE = LP_TRANSLATIONS_PAGE % (distro_id, distro_codename, source_package)
+    logging.debug("Open URL: {0}".format(URL_TEMPLATE))
+    
+    page = opener.open(URL_TEMPLATE)
     data = page.read()
 
     #templates = RE_TEMPLATE.findall(data)
@@ -76,7 +79,7 @@
     translation templates available in Launchpad"""
     parser = optparse.OptionParser(usage = USAGE)
     parser.add_option('-d', '--debug', dest='debug_mode', action='store_true',
-        help='Print the maximum debugging info (implies -vv)')
+        help='Print the maximum debugging info (implies -vvv)')
     parser.add_option('-v', '--verbose', dest='logging_level', action='count',
         help='Set error_level output to warning, info, and then debug')
     parser.add_option("-c", "--distro-codename", dest="distro_codename",
@@ -92,9 +95,11 @@
         options.logging_level = 3
     logging.basicConfig(level=LEVELS[options.logging_level], format='%(asctime)s %(levelname)s %(message)s')
 
+    # TODO: change to args[0] instead of -s
     if not options.source_package:
         print "Must specify a source package"
     else:
+        logging.debug("Source package: {0}".format(options.source_package))
         templates = templates_get(DISTRO_ID, options.distro_codename, options.source_package)
         if templates:
             for template in templates:

=== modified file 'ul10n_tools/lp_l10n_upload/__init__.py'
--- ul10n_tools/lp_l10n_upload/__init__.py	2011-05-30 08:41:38 +0000
+++ ul10n_tools/lp_l10n_upload/__init__.py	2011-05-31 08:16:39 +0000
@@ -20,8 +20,8 @@
 import tempfile, atexit, shutil, os.path
 import logging
 import optparse
-from ul10n_tools import ul10n_toolsconfig
 from ul10n_tools.utils import MultipartPostHandler
+from ul10n_tools.utils.launchpadmanager import get_launchpad
 import urllib2
 import cookielib
 import os
@@ -36,7 +36,7 @@
             logging.DEBUG,
             )
 
-from ul10n_tools.launchpadmanager import get_launchpad
+
 
 
 def opener_with_cookie(cookie_file):
@@ -131,7 +131,7 @@
                         format='%(asctime)s %(levelname)s %(message)s')
 
     if not options.distro_codename:
-        launchpad = getLaunchpad()
+        launchpad = get_launchpad()
         ubuntu = launchpad.distributions["ubuntu"]
         options.distro_codename = ubuntu.current_series.name
         logging.info("Ubuntu current series: {0}".format(options.distro_codename))
@@ -182,7 +182,7 @@
         html_page = response.read()
 
         with open('lp-response.html', 'w') as html_file:
-    	    html_file.write(html_page)
+            html_file.write(html_page)
     except IOError, e:
         print 'The "%s" page could not be opened.' % form_url
         if hasattr(e, 'code'):

=== modified file 'ul10n_tools/lp_set_pot_priority/__init__.py' (properties changed: -x to +x)
--- ul10n_tools/lp_set_pot_priority/__init__.py	2011-05-26 13:48:10 +0000
+++ ul10n_tools/lp_set_pot_priority/__init__.py	2011-05-31 08:16:39 +0000
@@ -15,16 +15,55 @@
 # the templates from within already exposed objects, instead of using 
 # "launchpad.load()" directly
 
-# TODO: import the launchpadmanager module from ul10n_tools and use that
-from launchpadlib.launchpad import Launchpad
+import sys
+import logging
+import optparse
+from ul10n_tools.utils.launchpadmanager import get_launchpad
 
+LEVELS = (  logging.ERROR,
+            logging.WARNING,
+            logging.INFO,
+            logging.DEBUG,
+            )
+            
 def main():
-    # TODO: make the Launchpad server ('production', 'staging') a command line option,
-    # (e.g. --use-production) making the default 'staging'
-    launchpad = Launchpad.login_with('Template priority setter', 'staging')
-
-    # TODO: make the file name the argument to the program invokation
-    template_list = open('natty-templates.csv').readlines()
+    # Support for command line options.
+    USAGE = """%prog [OPTIONS] pattern
+
+    Sets the priorities of translatable templates in Launchpad according to the 
+    given CSV file""" 
+    parser = optparse.OptionParser(usage = USAGE)
+    parser.add_option('-d', '--debug', dest='debug_mode', action='store_true',
+        help = 'Print the maximum debugging info (implies -vvv)')
+    parser.add_option('-v', '--verbose', dest='logging_level', action='count',
+        help = 'Set error_level output to warning, info, and then debug')
+    parser.add_option('-p', '--use-production', dest='use_staging', action='store_false',
+        help = 'Use the launchpad "production" server. Default is "staging"')
+
+    parser.set_defaults(logging_level = 0, use_staging = True)
+    (options, args) = parser.parse_args()   
+    
+    # Set the verbosity
+    if options.debug_mode:
+        options.logging_level = 3
+    logging.basicConfig(level = LEVELS[options.logging_level],
+                        format = '%(asctime)s %(levelname)s %(message)s')
+                        
+    # Error checking (the pattern must be specified)
+    if len(args) < 1:
+        print >> sys.stderr, 'ERROR: Not enough arguments, CSV file must be specified'
+        sys.exit(1)
+    else:
+        logging.debug("Using CSV file: %s" % (args[0]))
+    
+    logging.debug("Use staging: %s" % (options.use_staging))
+
+    launchpad = get_launchpad(options.use_staging)
+    
+    #launchpad = Launchpad.login_with('Template priority setter', launchpad_server)
+    launchpad = get_launchpad(options.use_staging)
+    
+    template_list = open(args[0]).readlines()
 
     for line in template_list[1:]:
         # TODO: use the csv module for reading the file
@@ -36,3 +75,4 @@
             sp + '/+pots/' + template)
         pot.priority = int(priority)
         pot.lp_save()
+        

=== modified file 'ul10n_tools/search/TranslationsSearch.py'
--- ul10n_tools/search/TranslationsSearch.py	2011-05-23 14:48:55 +0000
+++ ul10n_tools/search/TranslationsSearch.py	2011-05-31 08:16:39 +0000
@@ -22,7 +22,13 @@
 import os
 import sys
 import logging
-import polib
+
+try:
+    import polib
+except ImportError:
+    print >> sys.stderr, 'You need python-polib: \nsudo apt-get install python-polib'
+    sys.exit(1)
+
 
 def build_localedir(langcode, get_localedir_ubuntu = True):
     LOCALEDIR_ROOT = os.path.join(sys.prefix, 'share', 'locale')
@@ -61,48 +67,35 @@
             for entry in mo.translated_entries():
                 if entry.msgid_plural:
                     if self.mode == 'translations':
-                        entry_to_search = entry.msgstr_plural
-                        indexes = entry.msgstr_plural.keys()
+                        entries_to_search = entry.msgstr_plural.values()
                     elif self.mode == 'original':
-                        entry_to_search = entry.msgid_plural
-                        indexes = (entry.msgid, entry.msgid_plural)
+                        entries_to_search = [entry.msgid, entry.msgid_plural]
+                    elif self.mode == 'both':
+                        entries_to_search = [entry.msgid, entry.msgid_plural]
+                        entries_to_search.extend(entry.msgstr_plural.values())
                     else:
-                        # TODO: implement the 'both' mode
-                        print "Invalid mode specified:", options.mode
+                        print >> sys.stderr, "Invalid mode specified:", options.mode
                         sys.exit(1)
-
-                    for index in indexes:
-                        try:
-                            entry_to_search = entry_to_search[index]
-                        except TypeError:
-                            entry_to_search = index
-                        if self.pattern.search(entry_to_search):
-                            self.__add_match(mofile,
-                                        msgid = entry.msgid,
-                                        msgid_plural = entry.msgid_plural,
-                                        msgstr_plural = entry.msgstr_plural,
-                                        )
-                            break
+                    
                 else:
                     if self.mode == 'translations':
-                        entry_to_search = entry.msgstr
+                        entries_to_search = [entry.msgstr]
                     elif self.mode == 'original':
-                        entry_to_search = entry.msgid
+                        entries_to_search = [entry.msgid]
+                    elif self.mode == 'both':
+                        entries_to_search = [entry.msgstr, entry.msgid]
                     else:
-                        # TODO: implement the 'both' mode
-                        print "Invalid mode specified:", options.mode
+                        print >> sys.stderr, "Invalid mode specified:", self.mode
                         sys.exit(1)
-
+                        
+                for entry_to_search in entries_to_search:
                     if self.pattern.search(entry_to_search):
-                        self.__add_match(mofile,
-                                         msgid = entry.msgid,
-                                         msgstr = entry.msgstr)
+                        self.__add_match(mofile, entry)
+                        break
 
-    def __add_match(self, mofile, **kwargs):
+    def __add_match(self, mofile, entry):
         template_name = os.path.splitext(os.path.basename(mofile))[0]
 
-        entry = polib.POEntry(**kwargs)
-
         if template_name in self.matches:
             self.matches[template_name].append(entry)
         else:

=== modified file 'ul10n_tools/search/__init__.py'
--- ul10n_tools/search/__init__.py	2011-05-23 14:48:55 +0000
+++ ul10n_tools/search/__init__.py	2011-05-31 08:16:39 +0000
@@ -22,6 +22,7 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+import sys
 import logging
 import optparse
 import re
@@ -40,6 +41,8 @@
             logging.DEBUG,
             )
 
+SEARCH_MODES  = ('translations', 'original', 'both')
+
 def main():
     # Support for command line options.
     USAGE = """%prog [OPTIONS] pattern
@@ -48,7 +51,7 @@
     and returns any matches"""
     parser = optparse.OptionParser(usage = USAGE)
     parser.add_option('-d', '--debug', dest='debug_mode', action='store_true',
-        help = 'Print the maximum debugging info (implies -vv)')
+        help = 'Print the maximum debugging info (implies -vvv)')
     parser.add_option('-v', '--verbose', dest='logging_level', action='count',
         help = 'Set error_level output to warning, info, and then debug')
     parser.add_option("-l", "--language", dest="language",
@@ -58,33 +61,54 @@
         help = ("Specify the mode: 'translations' looks for the pattern " +
                 "in the translations; 'original' looks for the pattern " +
                 "in the original English messages; 'both' looks in both. " +
-                "('both' is not yet supported). " +
-                "The default is 'translations'."))
+                "The default is 'both'."))
+    """
+    # TODO Not implemented (yet)
     parser.add_option("-E", "--extended-regexp", dest="extended_regexp",
         action='store_true',
         help = "Specify that pattern is an extended regular expression")
-
+    """
     setlocale(LC_ALL, '')
 
     parser.set_defaults(logging_level = 0,
-                        mode = 'translations',
+                        mode = 'both',
                         language = getlocale()[0])
-    (options, args) = parser.parse_args()
-
+    (options, args) = parser.parse_args()   
+    
     # Set the verbosity
     if options.debug_mode:
         options.logging_level = 3
     logging.basicConfig(level = LEVELS[options.logging_level],
                         format = '%(asctime)s %(levelname)s %(message)s')
 
-    # TODO error checking (the pattern must be specified)
+    logging.debug("Search mode: {0}".format(options.mode))
+    logging.debug("Language: {0}".format(options.language))  
+    
+    # Error checking (the pattern must be specified)
+    if len(args) < 1:
+        print >> sys.stderr, 'ERROR: Not enough arguments, the search pattern must be specified'
+        sys.exit(1)
+    else:
+        logging.debug("Search string: {0}".format(args[0]))
+        
     pattern = re.compile(args[0])
-
-    # TODO error checking (check the localedir exists)
+    
     localedir = build_localedir(options.language)
-
-    # TODO error checking (check the mode has been specified)
-
+    logging.debug("Using localedir: {0}".format(localedir))
+    
+    # Error checking (check the localedir exists)
+    if localedir == None: 
+        print >> sys.stderr, 'ERROR: Could not locate directory for specified locale:', options.language
+        sys.exit(1)
+
+    # Error checking (check the mode that has been specified)    
+    if not options.mode in SEARCH_MODES: 
+        print >> sys.stderr, 'ERROR: Invalid mode specified:', options.mode        
+        print >> sys.stderr, "Valid modes are:",', '.join(SEARCH_MODES)
+        sys.exit(1)
+    
     translations_search = TranslationsSearch(pattern, localedir, options.mode)
     translations_search.search()
     translations_search.print_matches()
+    
+    print "{0} match(es) found.".format(len(translations_search.matches.items()))