Merge lp:~james-w/pkgme-devportal/better-api into lp:pkgme-devportal

[James Westby]

Add a more direct API for error handling and serialization of backends.

Hi,

Here's the start of an attempt to straighten out the APIs in pkgme-devportal.

This makes backend_script more directly reflect the API of a backend script
(though perhaps the path could be implicit as it's defined to be $PWD for
backends). It takes a function and just wraps it in the error handling
and serialization.

Next it removes the want() and all_info() functions because they are
specific to a particular way of writing a backend. In fact they move
to metadata.py and become function generators, which take a MetadataBackend
subclass and wrap it with the necessary stuff.

The rest is just changes to match this change.

I felt that this was a better organization, but it's not where I'll be stopping.
The stuff in metadata.py still assumes too much, and the MetadataBackend
class still does too much, but now I can change that to fit a better API
than before.

Thanks,

James

[Jonathan Lange (jml)]

Looks good to me.

I'm not sure why make_all_info_fn and make_want_fn wrap their returns in backend_script. Maybe that should be done in the scripts themselves? It doesn't really matter.

I look forward to seeing where this goes.

Thanks,
jml

review: Approve

[lp:~james-w/pkgme-devportal/better-api]

107. By James Westby on 2012-09-11

[r=james-w] Add a more direct API for error handling and serialization of backends.

=== modified file 'devportalbinary/backend.py'
--- devportalbinary/backend.py	2012-08-28 11:02:45 +0000
+++ devportalbinary/backend.py	2012-09-11 15:29:20 +0000
@@ -2,8 +2,8 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __all__ = [
-    'all_info',
-    'want',
+    'backend_script',
+    'convert_info',
     ]
 
 from functools import wraps
@@ -21,7 +21,7 @@
 BASE_USER_ERROR = PkgmeError
 
 
-def _convert_info(info):
+def convert_info(info):
     return dict((element.name, data) for element, data in info.items())
 
 
@@ -35,17 +35,18 @@
     error handling.  It takes care of converting the data to JSON and printing
     it out.
 
-    :param get_data: A function that returns a JSON-serializable object.
+    :param get_data: A function that returns a JSON-serializable object
+        when passed a path to inspect.
     :return: A function that can be invoked as the 'main' of a backend script.
     """
     @wraps(get_data)
-    def _wrapper(backend, output_stream=None, error_stream=None):
+    def _wrapper(path, output_stream=None, error_stream=None):
         if output_stream is None:
             output_stream = sys.stdout
         if error_stream is None:
             error_stream = sys.stderr
         try:
-            info = get_data(backend)
+            info = get_data(path)
         except BASE_USER_ERROR, e:
             error_stream.write(str(e))
             error_stream.write('\n')
@@ -54,15 +55,3 @@
             json.dump(info, output_stream)
             return 0
     return _wrapper
-
-
-@backend_script
-def all_info(backend):
-    metadata = backend.get_metadata()
-    info = backend.get_info(metadata)
-    return _convert_info(info)
-
-
-@backend_script
-def want(backend):
-    return backend.want()

=== modified file 'devportalbinary/backends/binary/all_info'
--- devportalbinary/backends/binary/all_info	2012-08-27 22:34:59 +0000
+++ devportalbinary/backends/binary/all_info	2012-09-11 15:29:20 +0000
@@ -2,11 +2,11 @@
 # Copyright 2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+import os
 import sys
 
-from devportalbinary.backend import all_info
-from devportalbinary.binary import BinaryBackend
+from devportalbinary.binary import all_info
 
 
 if __name__ == '__main__':
-    sys.exit(all_info(BinaryBackend()))
+    sys.exit(all_info(os.getcwd()))

=== modified file 'devportalbinary/backends/binary/want'
--- devportalbinary/backends/binary/want	2012-08-27 22:34:59 +0000
+++ devportalbinary/backends/binary/want	2012-09-11 15:29:20 +0000
@@ -2,11 +2,11 @@
 # Copyright 2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+import os
 import sys
 
-from devportalbinary.backend import want
-from devportalbinary.binary import BinaryBackend
+from devportalbinary.binary import want
 
 
 if __name__ == '__main__':
-    sys.exit(want(BinaryBackend()))
+    sys.exit(want(os.getcwd()))

=== modified file 'devportalbinary/backends/pdf/all_info'
--- devportalbinary/backends/pdf/all_info	2012-08-27 22:34:59 +0000
+++ devportalbinary/backends/pdf/all_info	2012-09-11 15:29:20 +0000
@@ -2,11 +2,11 @@
 # Copyright 2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+import os
 import sys
 
-from devportalbinary.backend import all_info
-from devportalbinary.pdf import PdfBackend
+from devportalbinary.pdf import all_info
 
 
 if __name__ == '__main__':
-    sys.exit(all_info(PdfBackend()))
+    sys.exit(all_info(os.getcwd()))

=== modified file 'devportalbinary/backends/pdf/want'
--- devportalbinary/backends/pdf/want	2012-08-27 22:34:59 +0000
+++ devportalbinary/backends/pdf/want	2012-09-11 15:29:20 +0000
@@ -2,11 +2,11 @@
 # Copyright 2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+import os
 import sys
 
-from devportalbinary.backend import want
-from devportalbinary.pdf import PdfBackend
+from devportalbinary.pdf import want
 
 
 if __name__ == '__main__':
-    sys.exit(want(PdfBackend()))
+    sys.exit(want(os.getcwd()))

=== modified file 'devportalbinary/binary.py'
--- devportalbinary/binary.py	2012-08-30 14:30:41 +0000
+++ devportalbinary/binary.py	2012-09-11 15:29:20 +0000
@@ -44,7 +44,11 @@
 
 from devportalbinary.configuration import load_configuration
 from devportalbinary.database import PackageDatabase
-from devportalbinary.metadata import MetadataBackend
+from devportalbinary.metadata import (
+    make_all_info_fn,
+    make_want_fn,
+    MetadataBackend,
+    )
 
 
 OVERRIDE_DH_STRIP_TEMPLATE = """
@@ -408,3 +412,7 @@
                 'reason': 'Has ELF binaries and a metadata file'}
         else:
             return {'score': 0, 'reason': 'No ELF binaries found.'}
+
+
+want = make_want_fn(BinaryBackend)
+all_info = make_all_info_fn(BinaryBackend)

=== modified file 'devportalbinary/metadata.py'
--- devportalbinary/metadata.py	2012-08-27 22:02:42 +0000
+++ devportalbinary/metadata.py	2012-09-11 15:29:20 +0000
@@ -46,6 +46,10 @@
     )
 from pkgme.project_info import DictInfo
 
+from .backend import (
+    backend_script,
+    convert_info,
+    )
 from .utils import get_latest_stable_ubuntu_distroseries
 
 
@@ -501,3 +505,21 @@
         Specific backends should override this.
         """
         raise NotImplementedError(self.want_with_metadata)
+
+
+def make_want_fn(backend_cls):
+    def metadata_want(path):
+        backend = backend_cls(path)
+        return backend.want()
+    metadata_want.__name__ = '{}_want'.format(backend_cls.__name__)
+    return backend_script(metadata_want)
+
+
+def make_all_info_fn(backend_cls):
+    def metadata_all_info(path):
+        backend = backend_cls(path)
+        metadata = backend.get_metadata()
+        info = backend.get_info(metadata)
+        return convert_info(info)
+    metadata_all_info.__name__ = '{}_all_info'.format(backend_cls.__name__)
+    return backend_script(metadata_all_info)

=== modified file 'devportalbinary/pdf.py'
--- devportalbinary/pdf.py	2012-08-23 12:09:47 +0000
+++ devportalbinary/pdf.py	2012-09-11 15:29:20 +0000
@@ -3,7 +3,11 @@
 
 import os
 
-from devportalbinary.metadata import MetadataBackend
+from devportalbinary.metadata import (
+    make_all_info_fn,
+    make_want_fn,
+    MetadataBackend,
+    )
 
 
 class PdfBackend(MetadataBackend):
@@ -66,3 +70,7 @@
         else:
             reason = 'More files than just a PDF: %r' % (sorted(files),)
         return {'score': score, 'reason': reason}
+
+
+want = make_want_fn(PdfBackend)
+all_info = make_all_info_fn(PdfBackend)

=== modified file 'devportalbinary/tests/test_backend.py'
--- devportalbinary/tests/test_backend.py	2012-08-28 11:02:45 +0000
+++ devportalbinary/tests/test_backend.py	2012-09-11 15:29:20 +0000
@@ -2,6 +2,7 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 import json
+import os
 from StringIO import StringIO
 
 from fixtures import MonkeyPatch
@@ -9,11 +10,10 @@
 from testtools import TestCase
 
 from devportalbinary.backend import (
-    all_info,
+    backend_script,
     BASE_USER_ERROR,
-    _convert_info,
+    convert_info,
     USER_ERROR_RETURN_CODE,
-    want,
     )
 
 
@@ -26,8 +26,8 @@
     def get_metadata(self):
         return self._metadata
 
-    def get_info(self, metadata):
-        return {PackageName: metadata}
+    def get_info(self, path):
+        return {PackageName.name: self._metadata}
 
     def want(self):
         return self._want
@@ -46,29 +46,29 @@
     def get_metadata(self):
         return {}
 
-    def get_info(self, metadata):
+    def get_info(self, path):
         raise self._raise()
 
     def want(self):
         raise self._raise()
 
 
-class TestDumpJSON(TestCase):
+class TestConvertInfo(TestCase):
 
     def test_convert(self):
         package_name = self.getUniqueString()
         info = {PackageName: package_name}
         self.assertEqual(
-            {PackageName.name: package_name}, _convert_info(info))
-
-
-class TestAllInfo(TestCase):
-
-    def test_all_info(self):
+            {PackageName.name: package_name}, convert_info(info))
+
+
+class TestBackendScript(TestCase):
+
+    def test_backend_script(self):
         metadata = {'foo': 'bar'}
         backend = DummyBackend(None, metadata)
         output = StringIO()
-        all_info(backend, output)
+        backend_script(backend.get_info)(os.getcwd(), output)
         self.assertEqual(
             {PackageName.name: metadata}, json.loads(output.getvalue()))
 
@@ -77,7 +77,7 @@
         self.useFixture(MonkeyPatch('sys.stdout', stream))
         metadata = {'foo': 'bar'}
         backend = DummyBackend(None, metadata)
-        all_info(backend)
+        backend_script(backend.get_info)(os.getcwd())
         self.assertEqual(
             {PackageName.name: metadata}, json.loads(stream.getvalue()))
 
@@ -85,61 +85,28 @@
         metadata = {'foo': 'bar'}
         backend = DummyBackend(None, metadata)
         output = StringIO()
-        result = all_info(backend, output)
-        self.assertEqual(0, result)
-
-    def test_normal_error_raises(self):
-        output = StringIO()
-        backend = BrokenBackend(RuntimeError, "Told you so")
-        self.assertRaises(RuntimeError, all_info, backend, output)
-
-    def test_user_error_logs(self):
-        output = StringIO()
-        error = StringIO()
-        backend = BrokenBackend(BASE_USER_ERROR, "Told you so")
-        result = all_info(backend, output, error)
-        self.assertEqual(USER_ERROR_RETURN_CODE, result)
-        self.assertEqual('', output.getvalue())
-        self.assertEqual("Told you so\n", error.getvalue())
-
-    def test_error_in_get_metatada(self):
-        output = StringIO()
-        error = StringIO()
-        backend = BrokenBackend(BASE_USER_ERROR, "Told you so")
-        backend.get_metadata = backend._raise
-        result = all_info(backend, output, error)
-        self.assertEqual(USER_ERROR_RETURN_CODE, result)
-        self.assertEqual('', output.getvalue())
-        self.assertEqual("Told you so\n", error.getvalue())
-
-
-class TestWant(TestCase):
-
-    def test_default_to_stdout(self):
-        stream = StringIO()
-        self.useFixture(MonkeyPatch('sys.stdout', stream))
-        want_reason = {'score': 10, 'reason': 'reason'}
-        backend = DummyBackend(want_reason, None)
-        want(backend)
-        self.assertEqual(want_reason, json.loads(stream.getvalue()))
-
-    def test_return_zero_on_success(self):
-        want_reason = {'score': 10, 'reason': 'reason'}
-        backend = DummyBackend(want_reason, None)
-        output = StringIO()
-        result = want(backend, output)
-        self.assertEqual(0, result)
-
-    def test_normal_error_raises(self):
-        output = StringIO()
-        backend = BrokenBackend(RuntimeError, "Told you so")
-        self.assertRaises(RuntimeError, want, backend, output)
-
-    def test_user_error_logs(self):
-        output = StringIO()
-        error = StringIO()
-        backend = BrokenBackend(BASE_USER_ERROR, "Told you so")
-        result = want(backend, output, error)
-        self.assertEqual(USER_ERROR_RETURN_CODE, result)
-        self.assertEqual('', output.getvalue())
-        self.assertEqual("Told you so\n", error.getvalue())
+        result = backend_script(backend.get_info)(os.getcwd(), output)
+        self.assertEqual(0, result)
+
+    def test_normal_error_raises(self):
+        output = StringIO()
+        backend = BrokenBackend(RuntimeError, "Told you so")
+        self.assertRaises(RuntimeError, backend_script(backend.get_info),
+                os.getcwd(), output)
+
+    def test_user_error_logs(self):
+        output = StringIO()
+        error = StringIO()
+        backend = BrokenBackend(BASE_USER_ERROR, "Told you so")
+        result = backend_script(backend.get_info)(os.getcwd(), output, error)
+        self.assertEqual(USER_ERROR_RETURN_CODE, result)
+        self.assertEqual('', output.getvalue())
+        self.assertEqual("Told you so\n", error.getvalue())
+
+    def test_passes_path(self):
+        output = StringIO()
+        expected_path = self.getUniqueString()
+        def return_path(path):
+            return path
+        result = backend_script(return_path)(expected_path, output)
+        self.assertEqual(expected_path, json.loads(output.getvalue()))

=== modified file 'devportalbinary/tests/test_metadata.py'
--- devportalbinary/tests/test_metadata.py	2012-08-27 22:02:42 +0000
+++ devportalbinary/tests/test_metadata.py	2012-09-11 15:29:20 +0000
@@ -1,6 +1,7 @@
 # Copyright 2011-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+from cStringIO import StringIO
 import json
 import os
 
@@ -36,6 +37,7 @@
     make_tree,
     )
 
+from devportalbinary.backend import convert_info
 from devportalbinary.metadata import (
     format_install_map,
     get_desktop_file,
@@ -43,6 +45,8 @@
     get_install_file,
     get_install_map,
     get_metadata,
+    make_all_info_fn,
+    make_want_fn,
     MetadataBackend,
     )
 from devportalbinary.testing import (
@@ -649,4 +653,39 @@
                 get_install_basedir(package_name))
 
 
+class TestMakeWantFn(TestCase):
+
+    def test_calls_want(self):
+        want = make_want_fn(DummyBackend)
+        path = self.getUniqueString()
+        backend = DummyBackend(path)
+        output = StringIO()
+        want(path, output)
+        self.assertEqual(backend.want(), json.loads(output.getvalue()))
+
+    def test_names_fn(self):
+        want = make_want_fn(DummyBackend)
+        self.assertEqual('DummyBackend_want', want.__name__)
+
+
+class TestMakeAllInfoFn(TestCase):
+
+    def test_calls_get_info(self):
+        class VeryDummyBackend(MetadataBackend):
+            def get_info(self, metadata):
+                return {PackageName: metadata}
+        expected_metadata = dict(foo="bar")
+        all_info = make_all_info_fn(VeryDummyBackend)
+        path = self.useFixture(MetadataFixture(expected_metadata)).path
+        backend = DummyBackend(path)
+        output = StringIO()
+        all_info(path, output)
+        self.assertEqual({PackageName.name: expected_metadata},
+                json.loads(output.getvalue()))
+
+    def test_names_fn(self):
+        all_info = make_all_info_fn(DummyBackend)
+        self.assertEqual('DummyBackend_all_info', all_info.__name__)
+
+
 # XXX: Assuming icon name in desktop == package name