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

[James Westby]

Move the want stuff to class methods and split some of the code out.

Hi,

This moves want() to be a classmethod, because it's part of a thing you
do before you use a backend. Maybe it should be moved from the class altogether.

It also moves some logic to a more generic function, and then uses that function.

It also drops some unused code (we never used the code to load a single field
from a metadata file, we just load the whole thing and use it as a dict.)

Thanks,

James

[Jonathan Lange (jml)]

Looks great. Dovetails nicely with what I'm doing.

load_json needs a proper docstring before landing though.

Thanks
jml

review: Approve

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

109. By James Westby on 2012-09-11

Better docstring. Thanks jml.

=== modified file 'devportalbinary/binary.py'
--- devportalbinary/binary.py	2012-09-11 15:21:46 +0000
+++ devportalbinary/binary.py	2012-09-11 16:56:39 +0000
@@ -405,8 +405,9 @@
         package_name = self.get_package_name(metadata)
         return guess_extra_debian_rules_targets(package_name, self.path)
 
-    def want_with_metadata(self, metadata):
-        if list(iter_binaries(self.path)):
+    @classmethod
+    def want_with_metadata(cls, path, metadata):
+        if list(iter_binaries(path)):
             return {
                 'score': 10,
                 'reason': 'Has ELF binaries and a metadata file'}

=== modified file 'devportalbinary/metadata.py'
--- devportalbinary/metadata.py	2012-09-11 15:21:46 +0000
+++ devportalbinary/metadata.py	2012-09-11 16:56:39 +0000
@@ -92,30 +92,6 @@
 }
 
 
-_no_field = object()
-def get_metadata(field_name=None, default=_no_field, path=None):
-    """Return the value of ``field_name`` in metadata.
-
-    :param field_name: The field to look up.  If None, then return all the
-        metadata as a dict.
-    :param default: If provided, then this value will be returned if
-        'field_name' is not present in the metadata.
-    :param path: The path to the metadata file, if unspecified, look for
-        ``MetadataBackend.METADATA_FILE`` in the current working directory.
-    :return: The value of the field.
-    """
-    if path is None:
-        path = MetadataBackend.METADATA_FILE
-    with open(path) as f:
-        metadata = json.load(f)
-    if field_name is None:
-        return metadata
-    value = metadata.get(field_name, default)
-    if value is _no_field:
-        raise KeyError(field_name)
-    return value
-
-
 def get_install_basedir(package_name):
     return '/opt/%s' % package_name
 
@@ -210,6 +186,25 @@
     return int(w)
 
 
+def load_json(path):
+    """Load JSON from `path`.
+
+    :param path: the path to load the JSON from.
+    :return: (json, err) where json is the deserialized contents of the file,
+        or None if it didn't exist or didn't contain json. If json is
+        None then err will be a description of why it couldn't be loaded.
+    """
+    if not os.path.exists(path):
+        return None, 'No metadata file'
+    try:
+        with open(path) as f:
+            metadata = json.load(f)
+    except ValueError:
+        # Not a JSON file.
+        return None, 'Metadata file is not valid JSON'
+    return metadata, None
+
+
 class MetadataBackend(object):
     """A backend that is mostly powered by metadata from MyApps."""
 
@@ -428,7 +423,12 @@
         """Get the version of the package."""
         return metadata.get(self.SUGGESTED_VERSION, None)
 
-    def get_metadata(self):
+    @classmethod
+    def get_metadata_path(cls, path):
+        return os.path.join(path, cls.METADATA_FILE)
+
+    @classmethod
+    def get_metadata(cls, path):
         """Get the metadata for this backend.
 
         Looks for the metadata in a file called ``METADATA_FILE`` in the
@@ -436,7 +436,10 @@
 
         :return: A dict of metadata.
         """
-        return get_metadata(path=self.metadata_path)
+        metadata, err = load_json(cls.get_metadata_path(path))
+        if metadata is None:
+            raise AssertionError(err)
+        return metadata
 
     def get_info(self, metadata):
         """Return a dict of InfoElements given 'metadata'.
@@ -484,22 +487,16 @@
             ExtraFiles, metadata, info[PackageName])
         return info
 
-    def has_metadata_file(self):
-        """True if a metadata file exists."""
-        return os.path.exists(self.metadata_path)
-
-    def want(self):
+    @classmethod
+    def want(cls, path):
         """How relevant this backend is."""
-        if not self.has_metadata_file():
-            return {'score': 0, 'reason': 'No metadata file'}
-        try:
-            metadata = self.get_metadata()
-        except ValueError:
-            # Not a JSON file.
-            return {'score': 0, 'reason': 'Metadata file is not valid JSON'}
-        return self.want_with_metadata(metadata)
+        metadata, err = load_json(cls.get_metadata_path(path))
+        if err is not None:
+            return {'score': 0, 'reason': err}
+        return cls.want_with_metadata(path, metadata)
 
-    def want_with_metadata(self, metadata):
+    @classmethod
+    def want_with_metadata(self, path, metadata):
         """How relevant this backend is, after metadata has been found.
 
         Specific backends should override this.
@@ -509,16 +506,15 @@
 
 def make_want_fn(backend_cls):
     def metadata_want(path):
-        backend = backend_cls(path)
-        return backend.want()
+        return backend_cls.want(path)
     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):
+        metadata = backend_cls.get_metadata(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__)

=== modified file 'devportalbinary/pdf.py'
--- devportalbinary/pdf.py	2012-09-11 15:21:46 +0000
+++ devportalbinary/pdf.py	2012-09-11 16:56:39 +0000
@@ -46,14 +46,15 @@
             return None
         return '/usr/bin/xdg-open /opt/%s/%s' % (package_name, pdf_filename)
 
-    def want_with_metadata(self, metadata):
+    @classmethod
+    def want_with_metadata(cls, path, metadata):
         # XXX: This implies that we're doing exact string matching on the
         # paths in the metadata and the results of os.listdir.  We actually
         # want to exclude the icons if they are the same file, not if the
         # strings happen to be equal.
-        excluded_files = set([self.METADATA_FILE, 'debian'])
-        excluded_files |= set(metadata.get(self.ICONS, {}).values())
-        files = os.listdir(self.path)
+        excluded_files = set([cls.METADATA_FILE, 'debian'])
+        excluded_files |= set(metadata.get(cls.ICONS, {}).values())
+        files = os.listdir(path)
         files = [f for f in files if f not in excluded_files]
         # By default, we don't want it and give no reason.  Sane default in
         # case of buggy code below.

=== modified file 'devportalbinary/tests/test_binary.py'
--- devportalbinary/tests/test_binary.py	2012-09-06 20:04:30 +0000
+++ devportalbinary/tests/test_binary.py	2012-09-11 16:56:39 +0000
@@ -293,20 +293,18 @@
         # top-level.
         path = self.useFixture(MetadataFixture({})).path
         self.useFixture(BinaryFileFixture(path))
-        backend = self.make_backend(path)
         self.assertEqual(
             {'score': 10,
              'reason': 'Has ELF binaries and a metadata file'},
-            backend.want())
+            BinaryBackend.want(path))
 
     def test_want_with_just_metadata(self):
         # If something just has a metadata file but no binaries it is
         # not wanted
         path = self.useFixture(MetadataFixture({})).path
-        backend = self.make_backend(path)
         self.assertEqual(
             {'score': 0,
-             'reason': 'No ELF binaries found.'}, backend.want())
+             'reason': 'No ELF binaries found.'}, BinaryBackend.want(path))
 
     def test_description(self):
         # The binary backend uses the package description that's in the

=== modified file 'devportalbinary/tests/test_metadata.py'
--- devportalbinary/tests/test_metadata.py	2012-09-11 15:21:46 +0000
+++ devportalbinary/tests/test_metadata.py	2012-09-11 16:56:39 +0000
@@ -37,14 +37,13 @@
     make_tree,
     )
 
-from devportalbinary.backend import convert_info
 from devportalbinary.metadata import (
     format_install_map,
     get_desktop_file,
     get_install_basedir,
     get_install_file,
     get_install_map,
-    get_metadata,
+    load_json,
     make_all_info_fn,
     make_want_fn,
     MetadataBackend,
@@ -80,10 +79,42 @@
     def get_executable(self, package_name):
         return 'executable:' + package_name
 
-    def want_with_metadata(self, metadata):
+    @classmethod
+    def want_with_metadata(self, path, metadata):
         return {'score': 20, 'reason': "I'm a dummy", 'metadata': metadata}
 
 
+class TestLoadJson(TestCase):
+
+    def test_load_json_all_fields(self):
+        metadata = {
+            'foo': self.getUniqueString(),
+            'bar': self.getUniqueInteger(),
+            }
+        path = self.useFixture(MetadataFixture(metadata)).metadata_path
+        found_metadata, err = load_json(path)
+        self.assertEqual(metadata, found_metadata)
+        self.assertEqual(None, err)
+
+    def test_load_json_not_present(self):
+        # load_json returns None and a description when the file is
+        # missing
+        filename = 'metdata.json'
+        path = self.useFixture(FileTree({})).join(filename)
+        found_metadata, err = load_json(path)
+        self.assertEqual(None, found_metadata)
+        self.assertEqual('No metadata file', err)
+
+    def test_load_json_not_json(self):
+        # load_json returns None and a description when the file
+        # does not contain json
+        filename = 'metdata.json'
+        path = self.useFixture(FileTree({filename: {}})).join(filename)
+        found_metadata, err = load_json(path)
+        self.assertEqual(None, found_metadata)
+        self.assertEqual('Metadata file is not valid JSON', err)
+
+
 class MetadataTests(BackendTests):
 
     BACKEND = DummyBackend
@@ -101,57 +132,6 @@
     def test_metadata_file(self):
         self.assertEqual('devportal-metadata.json', MetadataBackend.METADATA_FILE)
 
-    def test_get_metadata_field_present(self):
-        # get_metadata returns the metadata value of the requested field.
-        metadata = {
-            'foo': self.getUniqueString(),
-            'bar': self.getUniqueInteger(),
-            }
-        path = self.useFixture(MetadataFixture(metadata)).metadata_path
-        foo = get_metadata('foo', path=path)
-        self.assertEqual(metadata['foo'], foo)
-
-    def test_get_metadata_default_file(self):
-        # By default, get_metadata looks for the METADATA_FILE in the current
-        # working directory.
-        metadata = {
-            'foo': self.getUniqueString(),
-            'bar': self.getUniqueInteger(),
-            }
-        path = self.useFixture(MetadataFixture(metadata)).path
-        self.addCleanup(os.chdir, os.getcwd())
-        os.chdir(path)
-        foo = get_metadata('foo')
-        self.assertEqual(metadata['foo'], foo)
-
-    def test_get_metadata_field_not_present_default_provided(self):
-        # get_metadata returns the provided default value if the field is not
-        # present in the metadata.
-        metadata = {}
-        path = self.useFixture(MetadataFixture(metadata)).metadata_path
-        default = object()
-        foo = get_metadata('foo', default, path=path)
-        self.assertIs(default, foo)
-
-    def test_get_metadata_field_not_present_no_default(self):
-        # get_metadata raises an exception if the field isn't there and no
-        # default was provided.
-        metadata = {}
-        field = self.getUniqueString()
-        path = self.useFixture(MetadataFixture(metadata)).metadata_path
-        e = self.assertRaises(KeyError, get_metadata, field, path=path)
-        self.assertEqual(repr(field), str(e))
-
-    def test_get_metadata_all_fields(self):
-        # get_metadata returns the metadata value of the requested field.
-        metadata = {
-            'foo': self.getUniqueString(),
-            'bar': self.getUniqueInteger(),
-            }
-        path = self.useFixture(MetadataFixture(metadata)).metadata_path
-        found_metadata = get_metadata(path=path)
-        self.assertEqual(metadata, found_metadata)
-
     def test_get_extra_files_install_file(self):
         # ExtraFiles includes an install file generated by get_install_file.
         package_name = self.getUniqueString()
@@ -408,32 +388,30 @@
 
     def test_want_with_no_metadata(self):
         # If there's no metadata file, we don't want it.
-        backend = self.make_backend()
+        path = self.useFixture(TempDir()).path
         self.assertEqual(
             {'score': 0, 'reason': 'No metadata file'},
-            backend.want())
+            DummyBackend.want(path))
 
     def test_want_with_invalid_json(self):
         # If there's a metadata file, but it's not valid JSON, we don't want
         # it.
-        backend = self.make_backend()
-        make_tree(backend.path, from_rough_spec(
-            [(MetadataBackend.METADATA_FILE, 'not valid json')]))
+        path = self.useFixture(FileTree(from_rough_spec(
+            [(MetadataBackend.METADATA_FILE, 'not valid json')]))).path
         self.assertEqual(
             {'score': 0, 'reason': 'Metadata file is not valid JSON'},
-            backend.want())
+            DummyBackend.want(path))
 
     def test_want_with_valid_metadata(self):
         # If there's a metadata file containing valid JSON, we delegate to the
         # backend's want_with_metadata method.  See
         # DummyBackend.want_with_metadata.
-        backend = self.make_backend()
         metadata = {'foo': 'bar'}
-        make_tree(backend.path, from_rough_spec(
-            [(MetadataBackend.METADATA_FILE, json.dumps(metadata))]))
+        path = self.useFixture(FileTree(from_rough_spec(
+            [(MetadataBackend.METADATA_FILE, json.dumps(metadata))]))).path
         self.assertEqual(
             {'score': 20, 'reason': "I'm a dummy", 'metadata': metadata},
-            backend.want())
+            DummyBackend.want(path))
 
 
 class DesktopFileTests(TestCase):
@@ -661,7 +639,8 @@
         backend = DummyBackend(path)
         output = StringIO()
         want(path, output)
-        self.assertEqual(backend.want(), json.loads(output.getvalue()))
+        self.assertEqual(DummyBackend.want(path),
+                json.loads(output.getvalue()))
 
     def test_names_fn(self):
         want = make_want_fn(DummyBackend)

=== modified file 'devportalbinary/tests/test_pdf.py'
--- devportalbinary/tests/test_pdf.py	2012-08-23 12:05:12 +0000
+++ devportalbinary/tests/test_pdf.py	2012-09-11 16:56:39 +0000
@@ -27,27 +27,24 @@
         # If we detect the metadata file and a pdf, then we score 20.
         filename = self.getUniqueString() + ".pdf"
         path = self.make_tree([VALID_METADATA_FILE, filename])
-        backend = PdfBackend(path)
-        self.assertEqual({'score': 20, 'reason': None}, backend.want())
+        self.assertEqual({'score': 20, 'reason': None}, PdfBackend.want(path))
 
     def test_want_with_single_non_pdf(self):
         # If we detect a metadata file and a single other file that's not a
         # PDF, we score 0.
         filename = self.getUniqueString() + ".not-pdf"
         path = self.make_tree([VALID_METADATA_FILE, filename])
-        backend = PdfBackend(path)
         self.assertEqual(
             {'score': 0,
              'reason': 'File is not a PDF: %r' % (filename,),
-             }, backend.want())
+             }, PdfBackend.want(path))
 
     def test_want_with_just_metadata(self):
         # If we detect just the metadata file then we score 0.
         path = self.make_tree([VALID_METADATA_FILE])
-        backend = PdfBackend(path)
         self.assertEqual(
             {'score': 0, 'reason': 'No files found, just metadata'},
-            backend.want())
+            PdfBackend.want(path))
 
     def test_want_with_extra_files(self):
         # If we detect other files then we score 0. This is so that
@@ -56,19 +53,17 @@
         filename = self.getUniqueString() + ".pdf"
         other_filename = self.getUniqueString() + ".foo"
         path = self.make_tree([VALID_METADATA_FILE, filename, other_filename])
-        backend = PdfBackend(path)
         self.assertEqual(
             {'score': 0, 'reason': 'More files than just a PDF: %r' %
              (sorted([filename, other_filename]),)},
-            backend.want())
+            PdfBackend.want(path))
 
     def test_want_with_debian_dir(self):
         # If the other file is a debian dir then we score 20 still.
         # This just makes local testing of the backend easier.
         filename = self.getUniqueString() + ".pdf"
         path = self.make_tree([VALID_METADATA_FILE, filename, 'debian/'])
-        backend = PdfBackend(path)
-        self.assertEqual({'score': 20, 'reason': None}, backend.want())
+        self.assertEqual({'score': 20, 'reason': None}, PdfBackend.want(path))
 
     def test_want_with_icons(self):
         icon_file = self.getUniqueString() + '.png'
@@ -77,7 +72,7 @@
         path = self.make_tree([
             (MetadataBackend.METADATA_FILE, metadata), icon_file, filename])
         backend = PdfBackend(path)
-        self.assertEqual({'score': 20, 'reason': None}, backend.want())
+        self.assertEqual({'score': 20, 'reason': None}, PdfBackend.want(path))
 
     def test_architecture(self):
         # The pdf backend set architecture to all