Merge lp:~henninge/launchpad/bug-517080-pottery-split into lp:launchpad

[Henning Eggers]

Provided check for possible intltool structure in a bzrlib.Tree.

[Henning Eggers (henninge)]

= Bug 517080 =

Although the bug talks about "splitting up", this branch really provides an extra function that simply checks for "POTFILES.in" existing in the tree. There was no code that could be re-used because the existing code works on the filesystem while this new function works directly on a bzrlib.Tree object.

== Implementation details ==

The code is straight forward - walk the tree and report success if a "POTFILES.in" is found.

This makes use of the new "with" statement with a context manager to acquire a lock on the tree. This seems to be a perfect use case for this new language feature and eventually the context manager should become part of bzrlib.Tree.

== Tests ==

The new test case uses the same packages as the tests that ran on the filesystem but uses BzrDir to create a working tree from the directories. The files in the trees don't actually get added and committed to the tree, simply because for the function to work this is not necessary.

To share some functionality between test cases, a mix-in class was created and some methods moved into it. This bloats the diff quite a bit because the method names lose a leading underscore.

bin/test -vvt pottery

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/translations/pottery/detect_intltool.py
  lib/lp/translations/tests/test_pottery_detect_intltool.py

[Jeroen T. Vermeulen (jtv)]

Thanks for making the changes discussed in IRC. The new comments in the test are much clearer, and I think you've done a Good Thing™ in moving useTempDir up from TestCaseWithFactory into lp.testing.TestCase.

The code is good, with nice use of "with." Two more tiny things, both in the docstring for is_intltool_structure:

 * Say "whether" in this case, not "if." But that does make the line a bit long. Personally I prefer to phrase these descriptions as questions: "Does this source tree look like it's set up for intltool?"

 * Please make it state what "tree" is supposed to be.

review: Approve (code)

=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py	2010-02-03 19:29:27 +0000
+++ lib/lp/testing/__init__.py	2010-02-15 15:45:30 +0000
@@ -368,6 +368,14 @@
                 % (expected_count, len(statements), "\n".join(statements)))
         return ret
 
+    def useTempDir(self):
+        """Use a temporary directory for this test."""
+        tempdir = tempfile.mkdtemp()
+        self.addCleanup(lambda: shutil.rmtree(tempdir))
+        cwd = os.getcwd()
+        os.chdir(tempdir)
+        self.addCleanup(lambda: os.chdir(cwd))
+
 
 class TestCaseWithFactory(TestCase):
 
@@ -379,14 +387,6 @@
         self.factory = LaunchpadObjectFactory()
         self.real_bzr_server = False
 
-    def useTempDir(self):
-        """Use a temporary directory for this test."""
-        tempdir = tempfile.mkdtemp()
-        self.addCleanup(lambda: shutil.rmtree(tempdir))
-        cwd = os.getcwd()
-        os.chdir(tempdir)
-        self.addCleanup(lambda: os.chdir(cwd))
-
     def getUserBrowser(self, url=None):
         """Return a Browser logged in as a fresh user, maybe opened at `url`.
         """

=== modified file 'lib/lp/translations/pottery/detect_intltool.py'
--- lib/lp/translations/pottery/detect_intltool.py	2009-12-23 18:10:37 +0000
+++ lib/lp/translations/pottery/detect_intltool.py	2010-02-15 15:45:30 +0000
@@ -6,6 +6,8 @@
 """Functions to detect if intltool can be used to generate a POT file for the
 package in the current directory."""
 
+from __future__ import with_statement
+
 __metaclass__ = type
 __all__ = [
     'check_potfiles_in',
@@ -20,6 +22,39 @@
 from subprocess import call
 
 
+class ReadLockTree(object):
+    """Context manager to claim a read lock on a bzr tree."""
+
+    def __init__(self, tree):
+        self.tree = tree
+
+    def __enter__(self):
+        self.tree.lock_read()
+
+    def __exit__(self, exc_type, exc_val, exc_tb):
+        self.tree.unlock()
+        return False
+
+
+def is_intltool_structure(tree):
+    """Does this source tree look like it's set up for intltool?
+
+    Currently this just checks for the existence of POTFILES.in.
+
+    :param tree: A bzrlib.Tree object to search for the intltool structure.
+    :returns: True if signs of an intltool structure were found.
+    """
+    with ReadLockTree(tree):
+        for thedir, files in tree.walkdirs():
+            for afile in files:
+                file_path, file_name, file_type = afile[:3]
+                if file_type != 'file':
+                    continue
+                if file_name == "POTFILES.in":
+                    return True
+    return False
+
+
 def find_potfiles_in():
     """Search the current directory and its subdirectories for POTFILES.in.
 

=== modified file 'lib/lp/translations/tests/test_pottery_detect_intltool.py'
--- lib/lp/translations/tests/test_pottery_detect_intltool.py	2010-01-28 09:05:00 +0000
+++ lib/lp/translations/tests/test_pottery_detect_intltool.py	2010-02-15 15:45:30 +0000
@@ -10,59 +10,56 @@
 from StringIO import StringIO
 from textwrap import dedent
 
+from bzrlib.bzrdir import BzrDir
 from canonical.launchpad.scripts.tests import run_script
 from lp.translations.pottery.detect_intltool import (
     ConfigFile, check_potfiles_in, find_intltool_dirs, find_potfiles_in,
-    get_translation_domain)
+    get_translation_domain, is_intltool_structure)
 from lp.testing import TestCase
 
-class TestDetectIntltool(TestCase):
-
-    def setUp(self):
-        super(TestDetectIntltool, self).setUp()
-        # Determine test directory and create temporary working directory.
-        self.curdir = os.getcwd()
-        self.testdir = os.path.join(self.curdir, os.path.dirname(__file__))
-        self.workdir = tempfile.mkdtemp()
-        os.chdir(self.workdir)
-
-    def tearDown(self):
-        # Remove temporary directory.
-        os.chdir(self.curdir)
-        shutil.rmtree(self.workdir)
-        super(TestDetectIntltool, self).tearDown()
-
-    def _prepare_package(self, packagename):
-        # Unpack the specified pacakge to run the test against it.
-        # Change to this directory.
-        packagepath = os.path.join(self.testdir, packagename+".tar.bz2")
+
+class SetupTestPackageMixin(object):
+
+    def prepare_package(self, packagename):
+        """Unpack the specified package in a temporary directory.
+
+        Change into the package's directory.
+        """
+        # First build the path for the package.
+        packagepath = os.path.join(
+            os.getcwd(), os.path.dirname(__file__), packagename + ".tar.bz2")
+        # Then change into the temporary directory and unpack it.
+        self.useTempDir()
         tar = tarfile.open(packagepath, "r:bz2")
         tar.extractall()
         tar.close()
         os.chdir(packagename)
 
+
+class TestDetectIntltool(TestCase, SetupTestPackageMixin):
+
     def test_detect_potfiles_in(self):
         # Find POTFILES.in in a package with multiple dirs when only one has
         # POTFILES.in.
-        self._prepare_package("intltool_POTFILES_in_1")
+        self.prepare_package("intltool_POTFILES_in_1")
         dirs = find_potfiles_in()
         self.assertContentEqual(["./po-intltool"], dirs)
 
     def test_detect_potfiles_in_module(self):
         # Find POTFILES.in in a package with POTFILES.in at different levels.
-        self._prepare_package("intltool_POTFILES_in_2")
+        self.prepare_package("intltool_POTFILES_in_2")
         dirs = find_potfiles_in()
         self.assertContentEqual(["./po", "./module1/po"], dirs)
 
     def test_check_potfiles_in_content_ok(self):
         # Ideally all files listed in POTFILES.in exist in the source package.
-        self._prepare_package("intltool_single_ok")
+        self.prepare_package("intltool_single_ok")
         self.assertTrue(check_potfiles_in("./po")) 
 
     def test_check_potfiles_in_content_ok_file_added(self):
         # If a file is not listed in POTFILES.in, the file is still good for
         # our purposes.
-        self._prepare_package("intltool_single_ok")
+        self.prepare_package("intltool_single_ok")
         added_file = file("./src/sourcefile_new.c", "w")
         added_file.write("/* Test file. */")
         added_file.close()
@@ -72,53 +69,53 @@
         # If a file is missing that is listed in POTFILES.in, the file
         # intltool structure is probably broken and cannot be used for
         # our purposes.
-        self._prepare_package("intltool_single_ok")
+        self.prepare_package("intltool_single_ok")
         os.remove("./src/sourcefile1.c")
         self.assertFalse(check_potfiles_in("./po")) 
 
     def test_check_potfiles_in_wrong_directory(self):
         # Passing in the wrong directory will cause the check to fail
         # gracefully and return False.
-        self._prepare_package("intltool_single_ok")
+        self.prepare_package("intltool_single_ok")
         self.assertFalse(check_potfiles_in("./foo")) 
 
     def test_find_intltool_dirs(self):
         # Complete run: find all directories with intltool structure.
-        self._prepare_package("intltool_full_ok")
+        self.prepare_package("intltool_full_ok")
         self.assertEqual(
             ["./po-module1", "./po-module2"], find_intltool_dirs())
 
     def test_find_intltool_dirs_broken(self):
         # Complete run: part of the intltool structure is broken.
-        self._prepare_package("intltool_full_ok")
+        self.prepare_package("intltool_full_ok")
         os.remove("./src/module1/sourcefile1.c")
         self.assertEqual(
             ["./po-module2"], find_intltool_dirs())
 
     def test_get_translation_domain_makevars(self):
         # Find a translation domain in Makevars.
-        self._prepare_package("intltool_domain_makevars")
+        self.prepare_package("intltool_domain_makevars")
         self.assertEqual(
             "translationdomain",
             get_translation_domain("po"))
 
     def test_get_translation_domain_makefile_in_in(self):
         # Find a translation domain in Makefile.in.in.
-        self._prepare_package("intltool_domain_makefile_in_in")
+        self.prepare_package("intltool_domain_makefile_in_in")
         self.assertEqual(
             "packagename-in-in",
             get_translation_domain("po"))
 
     def test_get_translation_domain_configure_ac(self):
         # Find a translation domain in configure.ac.
-        self._prepare_package("intltool_domain_configure_ac")
+        self.prepare_package("intltool_domain_configure_ac")
         self.assertEqual(
             "packagename-ac",
             get_translation_domain("po"))
 
     def test_get_translation_domain_configure_in(self):
         # Find a translation domain in configure.in.
-        self._prepare_package("intltool_domain_configure_in")
+        self.prepare_package("intltool_domain_configure_in")
         self.assertEqual(
             "packagename-in",
             get_translation_domain("po"))
@@ -126,7 +123,7 @@
     def test_get_translation_domain_makefile_in_in_substitute(self):
         # Find a translation domain in Makefile.in.in with substitution from
         # configure.ac.
-        self._prepare_package("intltool_domain_makefile_in_in_substitute")
+        self.prepare_package("intltool_domain_makefile_in_in_substitute")
         self.assertEqual(
             "domainname-ac-in-in",
             get_translation_domain("po"))
@@ -135,7 +132,7 @@
         # Find a translation domain in Makefile.in.in with substitution from
         # configure.ac from a variable with the same name as in
         # Makefile.in.in.
-        self._prepare_package(
+        self.prepare_package(
             "intltool_domain_makefile_in_in_substitute_same_name")
         self.assertEqual(
             "packagename-ac-in-in",
@@ -144,7 +141,7 @@
     def test_get_translation_domain_makefile_in_in_substitute_same_file(self):
         # Find a translation domain in Makefile.in.in with substitution from
         # the same file.
-        self._prepare_package(
+        self.prepare_package(
             "intltool_domain_makefile_in_in_substitute_same_file")
         self.assertEqual(
             "domain-in-in-in-in",
@@ -153,14 +150,14 @@
     def test_get_translation_domain_makefile_in_in_substitute_broken(self):
         # Find no translation domain in Makefile.in.in when the substitution
         # cannot be fulfilled.
-        self._prepare_package(
+        self.prepare_package(
             "intltool_domain_makefile_in_in_substitute_broken")
         self.assertIs(None, get_translation_domain("po"))
 
     def test_get_translation_domain_configure_in_substitute_version(self):
         # Find a translation domain in configure.in with Makefile-style
         # substitution from the same file.
-        self._prepare_package(
+        self.prepare_package(
             "intltool_domain_configure_in_substitute_version")
         self.assertEqual(
             "domainname-in42",
@@ -168,7 +165,7 @@
 
     def test_pottery_check_intltool_script(self):
         # Let the script run to see it works fine.
-        self._prepare_package("intltool_full_ok")
+        self.prepare_package("intltool_full_ok")
 
         return_code, stdout, stderr = run_script(
             'scripts/rosetta/pottery-check-intltool.py', [])
@@ -179,6 +176,31 @@
             """), stdout)
 
 
+class TestDetectIntltoolInBzrTree(TestCase, SetupTestPackageMixin):
+
+    def prepare_tree(self):
+        return BzrDir.create_standalone_workingtree(".")
+
+    def test_detect_intltool_structure(self):
+        # Detect a simple intltool structure.
+        self.prepare_package("intltool_POTFILES_in_1")
+        tree = self.prepare_tree()
+        self.assertTrue(is_intltool_structure(tree))
+
+    def test_detect_no_intltool_structure(self):
+        # If no POTFILES.in exists, no intltool structure is assumed.
+        self.prepare_package("intltool_POTFILES_in_1")
+        os.remove("./po-intltool/POTFILES.in")
+        tree = self.prepare_tree()
+        self.assertFalse(is_intltool_structure(tree))
+
+    def test_detect_intltool_structure_module(self):
+        # Detect an intltool structure in subdirectories.
+        self.prepare_package("intltool_POTFILES_in_2")
+        tree = self.prepare_tree()
+        self.assertTrue(is_intltool_structure(tree))
+
+
 class TestConfigFile(TestCase):
 
     def setUp(self):