Code review comment for lp:~jtv/launchpad/validate-translations-file

Hi Jeroen,

Thanks for this branch and new tool. It looks good except for a few minor things. r=bac, merge-conditional

> === added file 'lib/lp/translations/scripts/tests/test_validate_translations_file.py'
> --- lib/lp/translations/scripts/tests/test_validate_translations_file.py 1970-01-01 00:00:00 +0000
> +++ lib/lp/translations/scripts/tests/test_validate_translations_file.py 2010-01-05 17:14:16 +0000
> @@ -0,0 +1,133 @@
> +#! /usr/bin/python2.5
> +#
> +# Copyright 2010 Canonical Ltd. This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).

You win brownie points for getting the year right!

> +"""Test the validate-translations-file script."""
> +
> +import logging
> +import os.path
> +from textwrap import dedent
> +from unittest import TestCase, TestLoader
> +
> +from canonical.launchpad.ftests.script import run_script
> +
> +import lp.translations
> +
> +from lp.translations.scripts.validate_translations_file import (
> + UnknownFileType, ValidateTranslationsFile)
> +from lp.translations.utilities.tests.xpi_helpers import (
> + get_en_US_xpi_file_to_import)
> +
> +
> +class TestValidateTranslationsFile(TestCase):
> +
> + def _makeValidator(self, test_args=None):
> + """Produce a ValidateTranslationsFile."""
> + if test_args is None:
> + test_args = []
> + validator = ValidateTranslationsFile(test_args=test_args)
> + validator.logger.setLevel(logging.CRITICAL)
> + return validator
> +
> + def _strip(self, file_contents):
> + """Remove leading newlines & indentation from file_contents."""
> + return dedent(file_contents.strip())
> +
> + def _findTestData(self):
> + """Return base path to this test's test data."""
> + return os.path.join(
> + os.path.dirname(lp.translations.__file__),
> + 'scripts/tests/test-data')
> +
> + def test_validate_unknown(self):
> + # Unknown filename extensions result in UnknownFileType.
> + validator = self._makeValidator(['foo.bar'])

The argument list here is confusing. Does it serve any purpose? If
not please remove it.

> === added file 'lib/lp/translations/scripts/validate_translations_file.py'
> --- lib/lp/translations/scripts/validate_translations_file.py 1970-01-01 00:00:00 +0000
> +++ lib/lp/translations/scripts/validate_translations_file.py 2010-01-05 17:14:16 +0000
> @@ -0,0 +1,131 @@
> +# Copyright 2010 Canonical Ltd. This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +__metaclass__ = type
> +
> +__all__ = [
> + 'UnknownFileType',
> + 'ValidateTranslationsFile',
> + ]
> +
> +from cStringIO import StringIO
> +import logging
> +from optparse import OptionParser
> +import os.path
> +
> +from canonical.launchpad import scripts
> +from lp.translations.utilities.gettext_po_parser import POParser
> +from lp.translations.utilities.mozilla_xpi_importer import (
> + DtdFile, MozillaZipImportParser)
> +from lp.translations.utilities.xpi_manifest import XpiManifest
> +
> +
> +class UnknownFileType(Exception):
> + """File's type is not recognized."""
> +
> +
> +def validate_unknown_file_type(filename, content):
> + """Fail validation: unknown file type."""
> + raise UnknownFileType("Unrecognized file type for '%s'." % filename)
> +
> +
> +def validate_dtd(filename, content):
> + """Validate XPI DTD file."""
> + DtdFile(filename, filename, content)
> +
> +
> +def validate_po(filename, content):
> + """Validate a gettext PO or POT file."""
> + POParser().parse(content)
> +
> +
> +def validate_xpi(filename, content):
> + """Validate an XPI archive."""
> + MozillaZipImportParser(filename, StringIO(content))
> +
> +
> +def validate_xpi_manifest(filename, content):
> + """Validate XPI manifest."""
> + XpiManifest(content)
> +
> +
> +

Remove the one extra blank line.

> +class ValidateTranslationsFile:
> + """Parse translations files to see if they are well-formed."""
> +
> + name = 'validate-translations-file'
> +
> + validators = {
> + 'dtd': validate_dtd,
> + 'manifest': validate_xpi_manifest,
> + 'po': validate_po,
> + 'pot': validate_po,
> + 'xpi': validate_xpi,
> + }
> +
> + def __init__(self, test_args=None):
> + """Set up basic facilities, similar to `LaunchpadScript`."""
> + self.parser = OptionParser()
> + scripts.logger_options(self.parser, default=logging.INFO)
> + self.options, self.args = self.parser.parse_args(args=test_args)
> + self.logger = scripts.logger(self.options, self.name)
> +
> + def main(self):
> + """Validate file(s)."""
> + failures = 0
> + files = len(self.args)
> + self.logger.info("Validating %d file(s)." % files)
> +
> + for filename in self.args:
> + if not self._readAndValidate(filename):
> + failures += 1
> +
> + if failures == 0:
> + self.logger.info("OK.")
> + elif failures > 1:
> + self.logger.error("%d failures in %d files." % (failures, files))
> + elif files > 1:
> + self.logger.error("1 failure in %d files." % files)
> + else:
> + self.logger.error("Validation failed.")
> +
> + if failures == 0:
> + return 0
> + else:
> + return 1
> +
> + def _pickValidator(self, filename):
> + """Select the appropriate validator for a file."""
> + base, ext = os.path.splitext(filename)
> + if ext is not None and ext.startswith('.'):
> + ext = ext[1:]
> + return self.validators.get(ext, validate_unknown_file_type)

Nice.

> === added file 'scripts/rosetta/validate-translations-file.py'
> --- scripts/rosetta/validate-translations-file.py 1970-01-01 00:00:00 +0000
> +++ scripts/rosetta/validate-translations-file.py 2010-01-05 17:14:16 +0000
> @@ -0,0 +1,18 @@
> +#! /usr/bin/python2.5
> +# Copyright 2009 Canonical Ltd. This software is licensed under the

Oops, brownie_points -= 1

> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +# pylint: disable-msg=W0403
> +
> +__metaclass__ = type
> +
> +import _pythonpath
> +
> +import sys
> +
> +from lp.translations.scripts.validate_translations_file import (
> + ValidateTranslationsFile)
> +
> +
> +if __name__ == "__main__":
> + sys.exit(ValidateTranslationsFile().main())