Merge lp:~jml/pkgme/extra-files into lp:pkgme

[Jonathan Lange]

Add a new type of information called 'ExtraFiles', which is a dictionary that maps paths to contents.

It's kind of a hacky implementation, and I haven't tried very hard to make sure it's complete (e.g. I have no idea how an extra_files script would actually work). But it's at a good point to get feedback.

[James Westby (james-w)]

90 +class TestExtraFiles(TestCase):
91 +
92 + def test_name(self):
93 + # ExtraFiles information is provided by an 'extra_files' script.
94 + self.assertEqual('extra_files', ExtraFiles.name)
95 +
96 + def test_default(self):
97 + # By default, there are no extra files.
98 + self.assertEqual({}, ExtraFiles.default)

I didn't do these tests for everything else, as they just seemed like they
would make things harder to change without really testing anything that is
likely to be a problem. I'm interested if you disagree with that opinion?

56 - keys = [element.name for element in elements]
57 + # XXX: Kind of hacky. The thing is that the ExtraFiles element isn't
58 + # an informational element used by key files, but rather a way of
59 + # forcing extra files to be written.
60 + keys = [ExtraFiles.name] + [element.name for element in elements]
61 values = project_info.get_all(keys)
62 new_info = DictInfo(values)
63 files = []
64 for file_cls in self.files_cls:
65 files.append(file_cls.from_info(new_info))
66 + extra_files = values.get(ExtraFiles.name, {})
67 + for path, contents in extra_files.items():
68 + files.append(BasicFile(path, contents))

This, perhaps unsurprisingly gives me pause.

I would have expected an ExtraFilesHandler class to be used, rather than a BasicFile
class. This would have been added to the default group, and so avoided the ExtraFiles.name
special casing there.

This class would then have done the for "path, contents in extra_files.items()" bit when
it was asked to write itself out at the end.

Did that approach not suit?

Thanks,

James

review: Needs Information

[Jonathan Lange (jml)]

On Thu, Jul 14, 2011 at 2:31 PM, James Westby <email address hidden> wrote:
> Review: Needs Information
> 90      +class TestExtraFiles(TestCase):
> 91      +
> 92      +    def test_name(self):
> 93      +        # ExtraFiles information is provided by an 'extra_files' script.
> 94      +        self.assertEqual('extra_files', ExtraFiles.name)
> 95      +
> 96      +    def test_default(self):
> 97      +        # By default, there are no extra files.
> 98      +        self.assertEqual({}, ExtraFiles.default)
>
> I didn't do these tests for everything else, as they just seemed like they
> would make things harder to change without really testing anything that is
> likely to be a problem. I'm interested if you disagree with that opinion?
>

Not really. Just added them because they helped me think straight & fast.

> 56      -        keys = [element.name for element in elements]
> 57      +        # XXX: Kind of hacky.  The thing is that the ExtraFiles element isn't
> 58      +        # an informational element used by key files, but rather a way of
> 59      +        # forcing extra files to be written.
> 60      +        keys = [ExtraFiles.name] + [element.name for element in elements]
> 61               values = project_info.get_all(keys)
> 62               new_info = DictInfo(values)
> 63               files = []
> 64               for file_cls in self.files_cls:
> 65                   files.append(file_cls.from_info(new_info))
> 66      +        extra_files = values.get(ExtraFiles.name, {})
> 67      +        for path, contents in extra_files.items():
> 68      +            files.append(BasicFile(path, contents))
>
> This, perhaps unsurprisingly gives me pause.
>
> I would have expected an ExtraFilesHandler class to be used, rather than a BasicFile
> class. This would have been added to the default group, and so avoided the ExtraFiles.name
> special casing there.
>
> This class would then have done the for "path, contents in extra_files.items()" bit when
> it was asked to write itself out at the end.
>
> Did that approach not suit?
>

Perhaps I don't understand you. The way this method is written in
trunk assumes that each thing is one file (file.append, not
files.extend). There's currently no "Handler"-style class that knows
how to write itself, only write_file knows how to do that, and it's
not going to write multiple files.

Will continue conversation on IRC & post back here for posterity.

jml

[Jonathan Lange (jml)]

After chatting on IRC, james_w says he's ok with the branch landing as-is. I'll probably follow up with one that changes the interface so that files know how to write themselves, and then blow away the special-casing.

[lp:~jml/pkgme/extra-files]

70. By Jonathan Lange on 2011-07-14

Merge testing-helpers into extra-files

71. By Jonathan Lange on 2011-07-14

Merge testing-helpers into extra-files

72. By Jonathan Lange on 2011-07-14

Remove unwanted tests.

=== modified file 'pkgme/info_elements.py'
--- pkgme/info_elements.py	2011-07-11 15:55:05 +0000
+++ pkgme/info_elements.py	2011-07-14 14:02:31 +0000
@@ -145,3 +145,14 @@
     name = "version"
     description = "The version of the project."
     default = 0
+
+
+class ExtraFiles(InfoElement):
+
+    name = 'extra_files'
+    description = """Any extra files needed to build the package.
+
+    This is specified as a dictionary mapping filename to file contents.  The
+    filename must be relative to the directory being packaged.
+    """
+    default = {}

=== modified file 'pkgme/package_files.py'
--- pkgme/package_files.py	2011-07-04 02:49:19 +0000
+++ pkgme/package_files.py	2011-07-14 14:02:31 +0000
@@ -10,6 +10,7 @@
     DebhelperAddons,
     Depends,
     Description,
+    ExtraFiles,
     Homepage,
     Maintainer,
     PackageName,
@@ -23,6 +24,20 @@
 DEBIAN_DIR = "debian"
 
 
+class BasicFile(object):
+
+    def __init__(self, path, content, overwrite=True, elements=None):
+        self.path = path
+        self._content = content
+        self.overwrite = overwrite
+        if elements is None:
+            elements = []
+        self.elements = elements
+
+    def get_contents(self):
+        return self._content
+
+
 class PackageFile(object):
 
     overwrite = True
@@ -66,12 +81,18 @@
 
     def get_files(self, project_info):
         elements = self.get_elements()
-        keys = [element.name for element in elements]
+        # XXX: Kind of hacky.  The thing is that the ExtraFiles element isn't
+        # an informational element used by key files, but rather a way of
+        # forcing extra files to be written.
+        keys = [ExtraFiles.name] + [element.name for element in elements]
         values = project_info.get_all(keys)
         new_info = DictInfo(values)
         files = []
         for file_cls in self.files_cls:
             files.append(file_cls.from_info(new_info))
+        extra_files = values.get(ExtraFiles.name, {})
+        for path, contents in extra_files.items():
+            files.append(BasicFile(path, contents))
         return files
 
     def get_elements(self):

=== modified file 'pkgme/tests/test_info_elements.py'
--- pkgme/tests/test_info_elements.py	2011-07-11 15:55:05 +0000
+++ pkgme/tests/test_info_elements.py	2011-07-14 14:02:31 +0000
@@ -2,6 +2,7 @@
 
 from pkgme.info_elements import (
     BuildDepends,
+    ExtraFiles,
     InfoElement,
     MissingInfoError,
     )

=== modified file 'pkgme/tests/test_package_files.py'
--- pkgme/tests/test_package_files.py	2011-07-14 10:59:20 +0000
+++ pkgme/tests/test_package_files.py	2011-07-14 14:02:31 +0000
@@ -11,6 +11,7 @@
     DebhelperAddons,
     Depends,
     Description,
+    ExtraFiles,
     Homepage,
     InfoElement,
     Maintainer,
@@ -19,6 +20,7 @@
     Version,
     )
 from pkgme.package_files import (
+    BasicFile,
     Changelog,
     Compat,
     Control,
@@ -68,6 +70,30 @@
         return self.cls.from_info(DictInfo(args))
 
 
+class BasicFileTests(TestCase):
+
+    def test_basic_file(self):
+        f = BasicFile('foo/bar.txt', 'content')
+        self.assertEqual('foo/bar.txt', f.path)
+        self.assertEqual('content', f.get_contents())
+
+    def test_default_overwrite(self):
+        f = BasicFile('foo/bar.txt', 'content')
+        self.assertEqual(True, f.overwrite)
+
+    def test_specified_overwrite(self):
+        f = BasicFile('foo/bar.txt', 'content', overwrite=False)
+        self.assertEqual(False, f.overwrite)
+
+    def test_default_elements(self):
+        f = BasicFile('foo/bar.txt', 'content')
+        self.assertEqual([], f.elements)
+
+    def test_specified_elements(self):
+        f = BasicFile('foo/bar.txt', 'content', elements=[TestElement2])
+        self.assertEqual([TestElement2], f.elements)
+
+
 class PackageFileTests(TestCase):
 
     def test_empty_by_default(self):
@@ -379,3 +405,16 @@
         group.add_file_cls(TestPackageFile)
         self.assertEqual(
             set([TestElement1, TestElement2]), group.get_elements())
+
+    def test_extra_files(self):
+        group = PackageFileGroup()
+        project_info = DictInfo({
+            ExtraFiles.name: {
+                'foo.txt': "Hello world!\n",
+                'debian/install': "Bwawahahaha\n",
+                }})
+        files = group.get_files(project_info)
+        [foo, install] = files
+        self.assertEqual('foo.txt', foo.path)
+        self.assertEqual("Hello world!\n", foo.get_contents())
+        self.assertEqual(True, foo.overwrite)