Merge lp:~jml/pkgme/format-description into lp:pkgme

[Jonathan Lange]

Auto-format description when it doesn't appear to be correctly formatted.

Gives Description powers of automatically formatting incoming descriptions so they fit into the Debian control file.

Implementation has been guided by http://www.debian.org/doc/debian-policy/ch-controlfields.html#s-f-Description, which I assume is the relevant standard.

In distinguishing between auto-formatted descriptions and ones that we leave alone, I felt it appropriate to add error checking for the ones we leave as-is. This means that callers of Description.clean() can rely on getting a proper description.

I'm raising user-friendly errors when the description is invalid, but I'm not 100% sure whether there would be a more appropriate way of raising them.

As an alternative strategy, when we find errors we could instead go back and treat the whole string as one that needs auto-formatting.

[James Westby (james-w)]

Hi,

I think this test shows undesirable behaviour:

162 + def test_non_indented_further_lines(self):

In the binary backend we're going to want to take the description from MyApps
and hand it over to this code.

If the developer enters a space at the start of the first line then it will
error, and there's nothing we can do except lstrip() the description.

I think that a better check for whether to act on it is whether there is
a space at the start of every line. You would still get in to trouble if
the developer put a space at the start of every line but left blank lines,
but that will be rarer.

I think this is what you meant by

  As an alternative strategy, when we find errors we could instead go back and treat the whole string as one that needs auto-formatting.

but I'm of course willing to be convinced otherwise.

Thanks,

James

review: Needs Fixing

[lp:~jml/pkgme/format-description]

102. By Jonathan Lange on 2012-01-09

Don't raise an error on unindented lines.

103. By Jonathan Lange on 2012-01-09

Just fix up wrong blank lines too.

[Jonathan Lange (jml)]

On Wed, Dec 21, 2011 at 4:23 PM, James Westby <email address hidden> wrote:
> Review: Needs Fixing
>
> Hi,
>
> I think this test shows undesirable behaviour:
>
> 162     + def test_non_indented_further_lines(self):
>
> In the binary backend we're going to want to take the description from MyApps
> and hand it over to this code.
>
> If the developer enters a space at the start of the first line then it will
> error, and there's nothing we can do except lstrip() the description.
>
> I think that a better check for whether to act on it is whether there is
> a space at the start of every line. You would still get in to trouble if
> the developer put a space at the start of every line but left blank lines,
> but that will be rarer.
>

I've changed this to not do a check at all, but to clean up the
description regardless. In this case, "clean" means to change any
blank lines to " ." and to indent any lines without indentation.

jml

[James Westby (james-w)]

Looks good to me, thanks for the changes.

review: Approve

=== modified file 'pkgme/info_elements.py'
--- pkgme/info_elements.py	2011-08-10 14:33:16 +0000
+++ pkgme/info_elements.py	2012-01-09 13:11:24 +0000
@@ -164,12 +164,61 @@
     description = """The dependencies of the binary package."""
 
 
+def chomp(text):
+    """Remove the trailing newline if present.
+
+    Named after the Perl built-in that does the same thing.
+    """
+    if text and text[-1] == '\n':
+        return text[:-1]
+    return text
+
+
 class Description(InfoElement):
 
     name = "description"
     description = "The description of the package."
     default = "a package"
 
+    @classmethod
+    def _format_lines(cls, lines):
+        first_line, lines = lines[0], lines[1:]
+        yield first_line
+        for i, line in enumerate(lines):
+            is_blank = bool(not line.strip())
+            if is_blank:
+                yield ' .'
+            elif line[0] == ' ':
+                yield line
+            else:
+                yield ' ' + line
+
+    @classmethod
+    def clean(cls, value):
+        """Prepare a string for the Description field in the control file.
+
+        If ``value`` looks to already be formatted according to Debian policy,
+        then we pass it through.  If it does not, then we do our best to clean
+        it up.
+
+        We determine whether ``value`` is already Debian-formatted by checking
+        whether the second line is indented by a space. If so, then we assume
+        it's Debian-formatted.  If later lines break Debian formatting, then
+        we raise a ValueError.
+
+        See http://www.debian.org/doc/debian-policy/ch-controlfields.html#s-f-Description
+        for the rules governing the Description field.
+
+        :param value: A string to go into the Description field of a control
+            file.
+        :raise ValueError: When the string initially looks to be Debian-
+            formatted but actually is not.
+        :return: A correctly-formatted version of that string.
+        """
+        if not value.strip():
+            return ''
+        return '\n'.join(cls._format_lines(value.strip().splitlines()))
+
 
 class Version(InfoElement):
 

=== modified file 'pkgme/tests/test_info_elements.py'
--- pkgme/tests/test_info_elements.py	2011-08-10 14:33:16 +0000
+++ pkgme/tests/test_info_elements.py	2012-01-09 13:11:24 +0000
@@ -4,6 +4,7 @@
 
 from pkgme.info_elements import (
     BuildDepends,
+    Description,
     Distribution,
     ExtraFiles,
     InfoElement,
@@ -132,3 +133,117 @@
                 line.strip().split('=') for line in fp.readlines())
         self.assertEqual(
             Distribution.get_default(), release_keys['DISTRIB_CODENAME'])
+
+
+class DescriptionTestCase(TestCase):
+
+    # http://www.debian.org/doc/debian-policy/ch-controlfields.html#s-f-Description
+
+    def test_single_line(self):
+        # If the description is just a single line with no line breaks, then
+        # that counts as the synopsis and no special formatting is needed.
+        description = "single line description"
+        cleaned = Description.clean(description)
+        self.assertEqual(description, cleaned)
+
+    def test_single_line_trailing_newline(self):
+        # If the single line ends with a newline then we remove that newline,
+        # as it's not needed for the Description in the template.
+        description = "single line description\n"
+        cleaned = Description.clean(description)
+        self.assertEqual(description.rstrip(), cleaned)
+
+    def test_indented_further_lines(self):
+        # If the incoming description is already in the format needed by the
+        # control file, then we do nothing to it.
+        description = """initial synopsis
+ follow-up line
+  preformatted text
+  more preformatted text
+ more information
+"""
+        cleaned = Description.clean(description)
+        self.assertEqual(description.rstrip(), cleaned)
+
+    def test_non_indented_further_lines(self):
+        # If the incoming description *looks* like it's in correct Debian
+        # format (i.e. has a leading space in the second line), then we assume
+        # that they don't know what they are doing and reformat the lines
+        # properly.
+        bad_description = """\
+initial synopsis
+ follow-up line
+more information
+"""
+        description = Description.clean(bad_description)
+        self.assertEqual("""\
+initial synopsis
+ follow-up line
+ more information""", description)
+
+    def test_bad_literal_description(self):
+        # If the incoming description *looks* like it's in correct Debian
+        # format (i.e. has a leading space in the second line), but then
+        # contains actual blank lines rather than lines with '.', then we
+        # raise an error.
+        bad_description = """initial synopsis
+ follow-up line
+
+ more information
+"""
+        description = Description.clean(bad_description)
+        self.assertEqual("""initial synopsis
+ follow-up line
+ .
+ more information""", description)
+
+    def test_multiple_lines(self):
+        # If the description appears to be out of the Debian format and spans
+        # multiple lines, then we indent those lines by a single space,
+        # treating them as paragraph text that will be word-wrapped.
+        description = """initial synopsis
+follow-up line
+more information
+"""
+        cleaned = Description.clean(description)
+        self.assertEqual("""initial synopsis
+ follow-up line
+ more information""", cleaned)
+
+    def test_blank_lines(self):
+        # If the description appears to be out of the Debian format and
+        # contains blank lines, we mark those blank lines with a single '.'.
+        description = """initial synopsis
+follow-up line
+
+more information
+
+that previous line has spaces in it, but needs to be marked
+up as a blank line all the same
+"""
+        cleaned = Description.clean(description)
+        self.assertEqual("""initial synopsis
+ follow-up line
+ .
+ more information
+ .
+ that previous line has spaces in it, but needs to be marked
+ up as a blank line all the same""", cleaned)
+
+    def test_empty_description(self):
+        # If the description is empty, then we return an empty description.
+        self.assertEqual('', Description.clean(''))
+        self.assertEqual('', Description.clean('\n'))
+        self.assertEqual('', Description.clean('  '))
+
+    def test_initial_blank_lines(self):
+        description = """
+
+initial synopsis
+follow-up line
+more information
+"""
+        cleaned = Description.clean(description)
+        self.assertEqual("""initial synopsis
+ follow-up line
+ more information""", cleaned)