Merge lp://staging/~niemeyer/lbox/long-summary into lp://staging/lbox

Please take a look.

Reviewers: mp+111099_code.launchpad.net,

Message:
Please take a look.

Description:
Prevent summaries from being over long.

https://code.launchpad.net/~niemeyer/lbox/long-summary/+merge/111099

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/6302100/

Affected files:
   A [revision details]
   M propose.go
   M submit.go
   M text.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: propose.go
=== modified file 'propose.go'
--- propose.go 2012-03-15 02:05:35 +0000
+++ propose.go 2012-06-19 20:04:41 +0000
@@ -238,6 +238,12 @@
     return
    }
   }
+ for newLog.SummaryTooLong() {
+ newLog, err = editLog(newLog)
+ if failed(err) {
+ return
+ }
+ }
   if !*proposeCr && newLog.CodeReviewId != 0 {
    // Branch is already linked to a codereview, so use -cr implicitly.
    *proposeCr = true

Index: submit.go
=== modified file 'submit.go'
--- submit.go 2012-03-15 15:31:12 +0000
+++ submit.go 2012-06-19 20:05:44 +0000
@@ -94,6 +94,12 @@
   if failed(err) {
    return
   }
+ for newLog.SummaryTooLong() {
+ newLog, err = editLog(newLog)
+ if failed(err) {
+ return
+ }
+ }
   message := newLog.String()

   // --- Update log and load data from Launchpad and Rietveld ---

Index: text.go
=== modified file 'text.go'
--- text.go 2012-02-08 01:30:18 +0000
+++ text.go 2012-06-19 20:09:16 +0000
@@ -24,6 +24,10 @@
   return t.Summary != other.Summary || t.Description != other.Description
  }

+func (t *Log) SummaryTooLong() bool {
+ return len(t.Summary) > 72
+}
+
  func (t *Log) String() string {
   if t.Description == "" {
    return t.Summary + "\n"
@@ -34,6 +38,7 @@
  const (
   templateSummary = "<enter change summary here>"
   templateDescription = "<enter change description here if sensible>"
+ templateTooLong = "[TOO LONG] "
  )

  func editLog(oldLog *Log) (newLog *Log, err error) {
@@ -52,6 +57,10 @@

   if oldLog == nil {
    _, err = fmt.Fprintf(f, "%s\n\n%s\n", templateSummary,
templateDescription)
+ } else if oldLog.SummaryTooLong() {
+ longLog := *oldLog
+ longLog.Summary = templateTooLong + longLog.Summary
+ _, err = fmt.Fprintf(f, "%s", &longLog)
   } else {
    _, err = fmt.Fprintf(f, "%s", oldLog)
   }
@@ -73,6 +82,9 @@
   f.Seek(0, 0)
   data, err := ioutil.ReadAll(f)
   newLog = parseLog(data)
+ if strings.HasPrefix(newLog.Summary, templateTooLong) {
+ newLog.Summary = newLog.Summary[len(templateTooLong):]
+ }
   if newLog.Summary == "" || newLog.Summary == templateSummary {
    return nil, fmt.Errorf("Change summary is empty.")
   }

*** Submitted:

Prevent summaries from being over long.

R=
CC=
https://codereview.appspot.com/6302100

https://codereview.appspot.com/6302100/

Thanks. Looks alright to my limited knowledge of the language (spell
while loops with 'for' huh?)

The different handling of long subject vs. no subject seems like a minor
wrinkle. Looping on Log validity instead might be neater, provided it is
still escapable. Giving a hint of how long might be nice, rather than
leaving it to trial and error for the user. (With git review I noticed I
get some vim highlight thing that changes colour at 50 characters, but
that's not in scope here.)

https://codereview.appspot.com/6302100/

Applied your suggestions on https://codereview.appspot.com/6305112

Except for the behavior on an empty log. This is a well known way to
cancel the action on that kind of software (bzr, git, etc), and I'm keen
on preserving that.

https://codereview.appspot.com/6302100/

> Except for the behavior on an empty log. This is a well known way to
cancel the
> action on that kind of software (bzr, git, etc), and I'm keen on
preserving
> that.

Thanks. What I was getting at is that, if I read the current logic
correctly, that the exit is triggered by an empty summary, not an empty
log - for instance a leading blank link.

https://codereview.appspot.com/6302100/