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

[Gustavo Niemeyer]

Prevent summaries from being over long.

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

[Gustavo Niemeyer (niemeyer)]

Please take a look.

[Gustavo Niemeyer (niemeyer)]

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.")
   }

[Gustavo Niemeyer (niemeyer)]

*** Submitted:

Prevent summaries from being over long.

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

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

[Martin Packman (gz)]

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/

[Gustavo Niemeyer (niemeyer)]

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/

[Martin Packman (gz)]

> 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/

=== modified file 'propose.go'
--- propose.go	2012-03-15 02:05:35 +0000
+++ propose.go	2012-06-19 20:11:18 +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

=== modified file 'submit.go'
--- submit.go	2012-03-15 15:31:12 +0000
+++ submit.go	2012-06-19 20:11:18 +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 ---

=== modified file 'text.go'
--- text.go	2012-02-08 01:30:18 +0000
+++ text.go	2012-06-19 20:11:18 +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.")
 	}