pkgme

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

  1. format-description
  2. Merge into trunk
Proposed by Jonathan Lange
Status: Merged
Approved by: James Westby
Approved revision: 103
Merged at revision: 94
Proposed branch: lp://staging/~jml/pkgme/format-description
Merge into: lp://staging/pkgme
Diff against target: 195 lines (+164/-0)
2 files modified
pkgme/info_elements.py (+49/-0)
pkgme/tests/test_info_elements.py (+115/-0)
To merge this branch: bzr merge lp://staging/~jml/pkgme/format-description
Reviewer Review Type Date Requested Status
James Westby Approve
Review via email: mp+86558@code.staging.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
James Westby (james-w) wrote :

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://staging/~jml/pkgme/format-description updated
102. By Jonathan Lange

Don't raise an error on unindented lines.

103. By Jonathan Lange

Just fix up wrong blank lines too.

Revision history for this message
Jonathan Lange (jml) wrote :

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

Revision history for this message
James Westby (james-w) wrote :

Looks good to me, thanks for the changes.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches