Repository: uber/prototool
Pull request #342 (2019-02-15 20:04:45)

 Split format.printer out to separate package
This makes it separately tested and usable by other packages. This also makes the `Printer` trim extra whitespace, which results in a small (cleanup) diff to format behavior.

codecov-io: # [Codecov](https://codecov.io/gh/uber/prototool/pull/342?src=pr&el=h1) Report
> Merging [#342](https://codecov.io/gh/uber/prototool/pull/342?src=pr&el=desc) into [dev](https://codecov.io/gh/uber/prototool/commit/6b375799c8fde1c6646fc57919018f99f886bbb8?src=pr&el=desc) will **increase** coverage by `0.03%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/uber/prototool/pull/342/graphs/tree.svg?width=650&token=A9V0DApqZg&height=150&src=pr)](https://codecov.io/gh/uber/prototool/pull/342?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##             dev     #342      +/-   ##
=========================================
+ Coverage   73.4%   73.43%   +0.03%     
=========================================
  Files        145      145              
  Lines       6422     6430       +8     
=========================================
+ Hits        4714     4722       +8     
  Misses      1271     1271              
  Partials     437      437
```


| [Impacted Files](https://codecov.io/gh/uber/prototool/pull/342?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [internal/format/base\_visitor.go](https://codecov.io/gh/uber/prototool/pull/342/diff?src=pr&el=tree#diff-aW50ZXJuYWwvZm9ybWF0L2Jhc2VfdmlzaXRvci5nbw==) | `84.9% <100%> (ø)` | :arrow_up: |
| [internal/buf/buf.go](https://codecov.io/gh/uber/prototool/pull/342/diff?src=pr&el=tree#diff-aW50ZXJuYWwvYnVmL2J1Zi5nbw==) | `100% <100%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/uber/prototool/pull/342?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/uber/prototool/pull/342?src=pr&el=footer). Last update [6b37579...777be97](https://codecov.io/gh/uber/prototool/pull/342?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/Makefile b/Makefile
index 05bc256..91537ba 100644
--- a/Makefile
+++ b/Makefile
@@ -56,6 +56,12 @@ golden: install
 	for file in $(shell find internal/cmd/testdata/format-fix -name '*.proto'); do \
 		prototool format --fix $${file} > $${file}.golden || true; \
 	done
+	for file in $(shell find internal/cmd/testdata/format-fix-v2 -name '*.proto.golden'); do \
+		rm -f $${file}; \
+	done
+	for file in $(shell find internal/cmd/testdata/format-fix-v2 -name '*.proto'); do \
+		prototool format --fix $${file} > $${file}.golden || true; \
+	done
 
 .PHONY: example
 example: install
diff --git a/internal/buf/BUILD.bazel b/internal/buf/BUILD.bazel
new file mode 100644
index 0000000..7272253
--- /dev/null
+++ b/internal/buf/BUILD.bazel
@@ -0,0 +1,15 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
+
+go_library(
+    name = "go_default_library",
+    srcs = ["buf.go"],
+    importpath = "github.com/uber/prototool/internal/buf",
+    visibility = ["//:__subpackages__"],
+)
+
+go_test(
+    name = "go_default_test",
+    srcs = ["buf_test.go"],
+    embed = [":go_default_library"],
+    deps = ["@com_github_stretchr_testify//assert:go_default_library"],
+)
diff --git a/internal/format/printer.go b/internal/buf/buf.go
similarity index 71%
rename from internal/format/printer.go
rename to internal/buf/buf.go
index 8d417cc..61885d8 100644
--- a/internal/format/printer.go
+++ b/internal/buf/buf.go
@@ -18,40 +18,47 @@
 // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
 // THE SOFTWARE.
 
-package format
+package buf
 
 import (
 	"bytes"
 	"fmt"
 	"strings"
+	"unicode"
 )
 
-const indentString = "  "
-
-// printer is a convenience struct that helps when printing proto files.
+// Printer is a convenience struct that helps when printing files.
+//
 // The concept was taken from the golang/protobuf plugin.
-type printer struct {
+type Printer struct {
+	indent      string
 	buffer      *bytes.Buffer
 	indentCount int
 }
 
-func newPrinter() *printer {
-	return &printer{bytes.NewBuffer(nil), 0}
+// NewPrinter returns a new Printer.
+func NewPrinter(indent string) *Printer {
+	return &Printer{
+		indent:      indent,
+		buffer:      bytes.NewBuffer(nil),
+		indentCount: 0,
+	}
 }
 
 // P prints the args concatenated on the same line after printing the current indent and then prints a newline.
-//
-// TODO: There is a lot of unnecessary memory allocation going on here, optimize
-func (p *printer) P(args ...interface{}) {
+func (p *Printer) P(args ...interface{}) {
+	if len(args) == 0 {
+		_, _ = p.buffer.WriteRune('\n')
+		return
+	}
 	lineBuffer := bytes.NewBuffer(nil)
 	if p.indentCount > 0 {
-		_, _ = fmt.Fprint(lineBuffer, strings.Repeat(indentString, p.indentCount))
+		_, _ = fmt.Fprint(lineBuffer, strings.Repeat(p.indent, p.indentCount))
 	}
 	for _, arg := range args {
 		_, _ = fmt.Fprint(lineBuffer, arg)
 	}
-	line := lineBuffer.Bytes()
-
+	line := bytes.TrimRightFunc(lineBuffer.Bytes(), unicode.IsSpace)
 	if len(bytes.TrimSpace(line)) != 0 {
 		_, _ = p.buffer.Write(line)
 	}
@@ -59,19 +66,24 @@ func (p *printer) P(args ...interface{}) {
 }
 
 // In adds one indent.
-func (p *printer) In() {
+func (p *Printer) In() {
 	p.indentCount++
 }
 
 // Out deletes one indent.
-func (p *printer) Out() {
+func (p *Printer) Out() {
 	// might want to error
 	if p.indentCount > 0 {
 		p.indentCount--
 	}
 }
 
+// String returns the printed string.
+func (p *Printer) String() string {
+	return p.buffer.String()
+}
+
 // Bytes returns the printed bytes.
-func (p *printer) Bytes() []byte {
+func (p *Printer) Bytes() []byte {
 	return p.buffer.Bytes()
 }
diff --git a/internal/format/BUILD.bazel b/internal/format/BUILD.bazel
index 77d737f..794eb7b 100644
--- a/internal/format/BUILD.bazel
+++ b/internal/format/BUILD.bazel
@@ -7,12 +7,12 @@ go_library(
         "first_pass_visitor.go",
         "format.go",
         "main_visitor.go",
-        "printer.go",
         "transformer.go",
     ],
     importpath = "github.com/uber/prototool/internal/format",
     visibility = ["//:__subpackages__"],
     deps = [
+        "//internal/buf:go_default_library",
         "//internal/protostrs:go_default_library",
         "//internal/text:go_default_library",
         "@com_github_emicklei_proto//:go_default_library",
diff --git a/internal/format/base_visitor.go b/internal/format/base_visitor.go
index 9830937..17c98bf 100644
--- a/internal/format/base_visitor.go
+++ b/internal/format/base_visitor.go
@@ -27,17 +27,18 @@ import (
 	"text/scanner"
 
 	"github.com/emicklei/proto"
+	"github.com/uber/prototool/internal/buf"
 	"github.com/uber/prototool/internal/text"
 )
 
 type baseVisitor struct {
-	*printer
+	*buf.Printer
 
 	Failures []*text.Failure
 }
 
 func newBaseVisitor() *baseVisitor {
-	return &baseVisitor{printer: newPrinter()}
+	return &baseVisitor{Printer: buf.NewPrinter("  ")}
 }
 
 func (v *baseVisitor) AddFailure(position scanner.Position, format string, args ...interface{}) {


Test patch:
diff --git a/internal/buf/buf_test.go b/internal/buf/buf_test.go
new file mode 100644
index 0000000..f873a93
--- /dev/null
+++ b/internal/buf/buf_test.go
@@ -0,0 +1,66 @@
+// Copyright (c) 2019 Uber Technologies, Inc.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in
+// all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+
+package buf
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestBasic(t *testing.T) {
+	testPrinter(
+		t,
+		"onetwothree\n",
+		func(p *Printer) {
+			p.P(`one`, `two`, `three`)
+		},
+	)
+	testPrinter(
+		t,
+		"one two three\n",
+		func(p *Printer) {
+			p.P(`one `, `two `, `three`)
+		},
+	)
+	testPrinter(
+		t,
+		"one\n  twothree\n   four\n\nfive\n\n",
+		func(p *Printer) {
+			// purposefully adding space
+			p.P(`one `)
+			p.In()
+			p.P(`two`, `three`)
+			// purposefully adding space
+			p.P(` four`)
+			p.Out()
+			p.P()
+			p.P(`five`)
+			p.P()
+		},
+	)
+}
+
+func testPrinter(t *testing.T, expected string, f func(*Printer)) {
+	printer := NewPrinter("  ")
+	f(printer)
+	assert.Equal(t, expected, printer.String())
+}
diff --git a/internal/cmd/testdata/format-fix-v2/foo.proto.golden b/internal/cmd/testdata/format-fix-v2/foo.proto.golden
index cdb546a..e609541 100644
--- a/internal/cmd/testdata/format-fix-v2/foo.proto.golden
+++ b/internal/cmd/testdata/format-fix-v2/foo.proto.golden
@@ -32,7 +32,7 @@ message Baz {
 
   // another unassociated comment
 
-  google.protobuf.Timestamp timestamp = 3; // inline c-style comment 
+  google.protobuf.Timestamp timestamp = 3; // inline c-style comment
   map<string, int64> m = 11;
   oneof test_oneof {
     int64 foo1 = 8;
diff --git a/internal/cmd/testdata/format-fix/foo.proto.golden b/internal/cmd/testdata/format-fix/foo.proto.golden
index 9b69f4f..7d9c6ab 100644
--- a/internal/cmd/testdata/format-fix/foo.proto.golden
+++ b/internal/cmd/testdata/format-fix/foo.proto.golden
@@ -29,7 +29,7 @@ message Baz {
 
   // another unassociated comment
 
-  google.protobuf.Timestamp timestamp = 3; // inline c-style comment 
+  google.protobuf.Timestamp timestamp = 3; // inline c-style comment
   map<string, int64> m = 11;
   oneof test_oneof {
     int64 foo1 = 8;
diff --git a/internal/cmd/testdata/format/proto3/foo/foo.proto.golden b/internal/cmd/testdata/format/proto3/foo/foo.proto.golden
index d6d3e17..d968b86 100644
--- a/internal/cmd/testdata/format/proto3/foo/foo.proto.golden
+++ b/internal/cmd/testdata/format/proto3/foo/foo.proto.golden
@@ -48,7 +48,7 @@ message Baz {
 
   // dep comment
   bar.Dep dep = 2;
-  google.protobuf.Timestamp timestamp = 3; // inline c-style comment 
+  google.protobuf.Timestamp timestamp = 3; // inline c-style comment
   int64 woot = 5 [(bar.field_option) = true];
   // comment17
   int64 woot2 = 6 [