Improve comment formatting (#152)

* Move addDocComment() into separate helper function

* Reformat comments

* Add blank lines before comment blocks

* Use Regexp to translate <br/>

* Use Index() and LastIndex() to simplify choosing breakpoints in lines

* Clarify comment

Co-authored-by: Dave Fellows <dave.fellows@microsoft.com>

* Fix comment

* Change method to internal, but document it anyway

* Fix search/replace typo & clarify comments

* Update golden files with modified comments

* Move definition of brRegex next to formatDocComment()

* Add missing file headers

Co-authored-by: Dave Fellows <dave.fellows@microsoft.com>
Co-authored-by: George Pollard <gpollard@microsoft.com>

hack/generator/pkg/astmodel/comments.go

@@ -0,0 +1,100 @@
+/*
+ * Copyright (c) Microsoft Corporation.
+ * Licensed under the MIT license.
+ */
+
+package astmodel
+
+import (
+	"go/ast"
+	"regexp"
+	"strings"
+)
+
+// Utility methods for adding comments
+
+func addDocComment(commentList *[]*ast.Comment, comment string, width int) {
+	for _, c := range formatDocComment(comment, width) {
+		line := strings.TrimSpace(c)
+		if !strings.HasPrefix(line, "//") {
+			line = "//" + line
+		}
+
+		if *commentList == nil {
+			line = "\n" + line
+		}
+
+		*commentList = append(*commentList, &ast.Comment{
+			Text: line,
+		})
+	}
+}
+
+// formatDocComment splits the supplied comment string up ready for use as a documentation comment
+func formatDocComment(comment string, width int) []string {
+	// Remove markdown bolding
+	text := strings.ReplaceAll(comment, "**", "")
+
+	// Turn <br> and <br/> into \n
+	text = brRegex.ReplaceAllLiteralString(text, "\n")
+
+	// Split into individual lines
+	lines := strings.Split(text, "\n")
+
+	// Trim whitespace
+	for i, l := range lines {
+		lines[i] = strings.TrimSpace(l)
+	}
+
+	// Wordwrap and return
+	return docCommentWrap(lines, width)
+}
+
+var brRegex = regexp.MustCompile("<br[^/>]*/?>")
+
+func docCommentWrap(lines []string, width int) []string {
+	var result []string
+	for _, l := range lines {
+		result = append(result, wordWrap(l, width)...)
+	}
+
+	return result
+}
+
+func wordWrap(text string, width int) []string {
+	var result []string
+
+	start := 0
+	for start < len(text) {
+		finish := findBreakPoint(text, start, width)
+		result = append(result, text[start:finish+1])
+		start = finish + 1
+	}
+
+	return result
+}
+
+// findBreakPoint finds the character at which to break two lines
+// Returned index points to the last character that should be included on the line
+// If breaking at a space, this will give a trailing space, but allows for
+// breaking at other points too as no characters will be omitted.
+func findBreakPoint(line string, start int, width int) int {
+	limit := start + width + 1
+	if limit >= len(line) {
+		return len(line) - 1
+	}
+
+	// Look for a word break within the line
+	index := strings.LastIndex(line[start:limit], " ")
+	if index >= 0 {
+		return start + index
+	}
+
+	// Line contains continuous text, we don't want to break it in two, so find the end of it
+	index = strings.Index(line[limit:], " ")
+	if index >= 0 {
+		return limit + index
+	}
+
+	return len(line) - 1
+}

hack/generator/pkg/astmodel/comments_test.go

@@ -0,0 +1,77 @@
+/*
+ * Copyright (c) Microsoft Corporation.
+ * Licensed under the MIT license.
+ */
+
+package astmodel
+
+import (
+	. "github.com/onsi/gomega"
+	"testing"
+)
+
+func TestDocumentationCommentFormatting(t *testing.T) {
+	cases := []struct {
+		comment string
+		results []string
+	}{
+		// Expect short single line comments to be unchanged
+		{"foo", []string{"foo"}},
+		{"bar", []string{"bar"}},
+		{"baz", []string{"baz"}},
+		// Leading and trailing whitespace is trimmed
+		{"    foo", []string{"foo"}},
+		{"foo    ", []string{"foo"}},
+		{"  foo  ", []string{"foo"}},
+		// Expect comments with embedded newlines to be split
+		{"foo\nbar", []string{"foo", "bar"}},
+		{"foo\nbar\nbaz", []string{"foo", "bar", "baz"}},
+		// Expect comments with html style <br> to be split
+		{"foo<br>bar", []string{"foo", "bar"}},
+		{"foo<br>bar<br>baz", []string{"foo", "bar", "baz"}},
+		{"foo<br/>bar", []string{"foo", "bar"}},
+		{"foo<br/>bar<br/>baz", []string{"foo", "bar", "baz"}},
+		// Expect markdown bold to be removed
+		{"**foo**\nbar", []string{"foo", "bar"}},
+		{"foo\n**bar**", []string{"foo", "bar"}},
+		{"foo\n**bar**\nbaz", []string{"foo", "bar", "baz"}},
+		// Expect long lines to be wrapped
+		{
+			"Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.",
+			[]string{
+				"Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do ",
+				"eiusmod tempor incididunt ut labore et dolore magna aliqua.",
+			}},
+	}
+
+	for _, c := range cases {
+		c := c
+		t.Run(c.comment, func(t *testing.T) {
+			g := NewGomegaWithT(t)
+			lines := formatDocComment(c.comment, 64)
+			g.Expect(lines).To(Equal(c.results))
+		})
+	}
+}
+
+func TestWordWrap(t *testing.T) {
+	cases := []struct {
+		text    string
+		width   int
+		results []string
+	}{
+		{"this is a simple line of text", 15, []string{"this is a ", "simple line of ", "text"}},
+		{"this is a simple line of text", 16, []string{"this is a simple ", "line of text"}},
+		{"this is a simple line of text", 20, []string{"this is a simple ", "line of text"}},
+		{"this is a simple line of text", 21, []string{"this is a simple line ", "of text"}},
+	}
+
+	for _, c := range cases {
+		c := c
+		t.Run(c.text, func(t *testing.T) {
+			g := NewGomegaWithT(t)
+			lines := wordWrap(c.text, c.width)
+			g.Expect(lines).To(Equal(c.results))
+		})
+	}
+}

hack/generator/pkg/astmodel/field_definition.go

@@ -107,27 +107,15 @@ func (field *FieldDefinition) AsField(codeGenerationContext *CodeGenerationConte
 		},
 	}
 
-	addDocComment := func(comment string) {
-		newLine := ""
-		if result.Doc.List == nil {
-			// if first comment, add a newline
-			newLine = "\n"
-		}
-
-		result.Doc.List = append(result.Doc.List, &ast.Comment{
-			Text: newLine + comment,
-		})
-	}
-
 	// generate validation comments:
 	for _, validation := range field.validations {
 		// these are not doc comments but they must go here to be emitted before the field
-		addDocComment(GenerateKubebuilderComment(validation))
+		addDocComment(&result.Doc.List, GenerateKubebuilderComment(validation), 200)
 	}
 
 	// generate doc comment:
 	if field.description != "" {
-		addDocComment(fmt.Sprintf("/*%s: %s*/", field.fieldName, field.description))
+		addDocComment(&result.Doc.List, fmt.Sprintf("%s: %s", field.fieldName, field.description), 80)
 	}
 
 	return result

hack/generator/pkg/astmodel/file_definition.go

@@ -6,6 +6,7 @@
 package astmodel
 
 import (
+	"bufio"
 	"bytes"
 	"go/ast"
 	"go/format"
@@ -14,6 +15,7 @@ import (
 	"io"
 	"os"
 	"sort"
+	"strings"
 
 	"k8s.io/klog/v2"
 )
@@ -175,21 +177,24 @@ func (file FileDefinition) SaveToWriter(filename string, dst io.Writer) error {
 	fset := token.NewFileSet()
 	fset.AddFile(filename, 1, 102400)
 
-	var buffer bytes.Buffer
-	err := format.Node(&buffer, fset, original)
+	var unformattedBuffer bytes.Buffer
+	err := format.Node(&unformattedBuffer, fset, original)
 	if err != nil {
 		return err
 	}
 
-	// Parse it out of the buffer again so we can "go fmt" it
-	var toFormat ast.Node
-	toFormat, err = parser.ParseFile(fset, filename, &buffer, parser.ParseComments)
+	// This is a nasty technique with only one redeeming characteristic: It works
+	reformattedBuffer := file.addBlankLinesBeforeComments(unformattedBuffer)
+
+	// Read the source from the memory buffer (has the effect similar to 'go fmt')
+	var cleanAst ast.Node
+	cleanAst, err = parser.ParseFile(fset, filename, &reformattedBuffer, parser.ParseComments)
 	if err != nil {
 		klog.Errorf("Failed to reformat code (%s); keeping code as is.", err)
-		toFormat = original
+		cleanAst = original
 	}
 
-	return format.Node(dst, fset, toFormat)
+	return format.Node(dst, fset, cleanAst)
 }
 
 // SaveToFile writes this generated file to disk
@@ -204,3 +209,35 @@ func (file FileDefinition) SaveToFile(filePath string) error {
 
 	return file.SaveToWriter(filePath, f)
 }
+
+// addBlankLinesBeforeComments reads the source in the passed buffer and injects a blank line just
+// before each '//' style comment so that the comments are nicely spaced out in the generated code.
+func (file FileDefinition) addBlankLinesBeforeComments(buffer bytes.Buffer) bytes.Buffer {
+	// Read all the lines from the buffer
+	var lines []string
+	reader := bufio.NewReader(&buffer)
+	scanner := bufio.NewScanner(reader)
+	for scanner.Scan() {
+		lines = append(lines, scanner.Text())
+	}
+
+	isComment := func(s string) bool {
+		return strings.HasPrefix(strings.TrimSpace(s), "//")
+	}
+
+	var result bytes.Buffer
+	lastLineWasComment := false
+	for _, l := range lines {
+		// Add blank line prior to each comment block
+		if !lastLineWasComment && isComment(l) {
+			result.WriteString("\n")
+		}
+
+		result.WriteString(l)
+		result.WriteString("\n")
+
+		lastLineWasComment = isComment(l)
+	}
+
+	return result
+}

hack/generator/pkg/astmodel/simple_type_definer.go

@@ -41,13 +41,8 @@ func (std *SimpleTypeDefiner) WithDescription(desc *string) TypeDefiner {
 func (std *SimpleTypeDefiner) AsDeclarations(codeGenerationContext *CodeGenerationContext) []ast.Decl {
 	var docComments *ast.CommentGroup
 	if std.description != nil {
-		docComments = &ast.CommentGroup{
-			List: []*ast.Comment{
-				{
-					Text: "\n/*" + *std.description + "*/",
-				},
-			},
-		}
+		docComments = &ast.CommentGroup{}
+		addDocComment(&docComments.List, *std.description, 120)
 	}
 
 	return []ast.Decl{

hack/generator/pkg/astmodel/struct_definition.go

@@ -81,8 +81,7 @@ func (definition *StructDefinition) AsDeclarations(codeGenerationContext *CodeGe
 	}
 
 	if definition.description != nil {
-		declaration.Doc.List = append(declaration.Doc.List,
-			&ast.Comment{Text: "\n/*" + *definition.description + "*/"})
+		addDocComment(&declaration.Doc.List, *definition.description, 200)
 	}
 
 	declarations := []ast.Decl{declaration}

hack/generator/pkg/jsonast/testdata/AdditionalProperties/Generates_field_if_other_fields_present.golden

@@ -5,7 +5,7 @@ package v20200101
 
 import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
-/*Generated from: https://test.test/schemas/2020-01-01/test.json*/
+//Generated from: https://test.test/schemas/2020-01-01/test.json
 type Test struct {
 	Tags *TestTags `json:"tags"`
 }

hack/generator/pkg/jsonast/testdata/AdditionalProperties/Generates_map[string]interface{}_if_unset_and_no_other_fields_present.golden

@@ -5,7 +5,7 @@ package v20200101
 
 import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
-/*Generated from: https://test.test/schemas/2020-01-01/test.json*/
+//Generated from: https://test.test/schemas/2020-01-01/test.json
 type Test struct {
 	Tags *map[string]interface{} `json:"tags"`
 }

hack/generator/pkg/jsonast/testdata/AdditionalProperties/Generates_map_type_if_no_other_fields_present.golden

@@ -5,7 +5,7 @@ package v20200101
 
 import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
-/*Generated from: https://test.test/schemas/2020-01-01/test.json*/
+//Generated from: https://test.test/schemas/2020-01-01/test.json
 type Test struct {
 	Tags *map[string]float64 `json:"tags"`
 }

hack/generator/pkg/jsonast/testdata/AdditionalProperties/Generates_nothing_if_set_to_'false'.golden

@@ -5,7 +5,7 @@ package v20200101
 
 import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
-/*Generated from: https://test.test/schemas/2020-01-01/test.json*/
+//Generated from: https://test.test/schemas/2020-01-01/test.json
 type Test struct {
 	Tags *TestTags `json:"tags"`
 }

hack/generator/pkg/jsonast/testdata/AdditionalProperties/Generates_nothing_if_unset_and_other_fields_present.golden

@@ -5,7 +5,7 @@ package v20200101
 
 import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
-/*Generated from: https://test.test/schemas/2020-01-01/test.json*/
+//Generated from: https://test.test/schemas/2020-01-01/test.json
 type Test struct {
 	Tags *TestTags `json:"tags"`
 }

hack/generator/pkg/jsonast/testdata/AllOf/AllOf_collapses_duplicate_named_types.golden

@@ -5,10 +5,10 @@ package v20200101
 
 import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
-/*Generated from: https://test.test/schemas/2020-01-01/test.json#/definitions/Foo*/
+//Generated from: https://test.test/schemas/2020-01-01/test.json#/definitions/Foo
 type Foo struct {
 	Name *string `json:"name"`
 }
 
-/*Generated from: https://test.test/schemas/2020-01-01/test.json*/
+//Generated from: https://test.test/schemas/2020-01-01/test.json
 type Test Foo

hack/generator/pkg/jsonast/testdata/AllOf/AllOf_collapses_duplicate_unnamed_types.golden

@@ -5,7 +5,7 @@ package v20200101
 
 import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
-/*Generated from: https://test.test/schemas/2020-01-01/test.json*/
+//Generated from: https://test.test/schemas/2020-01-01/test.json
 type Test struct {
 	Name *string `json:"name"`
 }