Code review comment for lp:~jml/pkgme/extra-files

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